Groundwork for v2

[gkats]

Type of change

• 🐛 Bug fix (non-breaking change which fixes an issue)
• 🌟 New feature (non-breaking change which adds functionality)
• 🔨 Breaking change (fix or feature that would cause existing functionality)
• 📖 Docs change / refactoring / dependency upgrade to change)

Description

🎉 Kicking off the v2 rewrite of our Go SDK. 🎉

The work here is split into three commits. It might be easier to review commit by commit, because there's a lot of changed files.

• First commit deletes all existing files.
• Second commit contains the basis for building API operations in our SDK.
• Third commit sets up a GH workflow for CI.

We're moving into a different direction with our SDK. This PR includes the core tools for implementing all API operations.

We have three basic interfaces in our SDK.

// Backend send API requests to the Clerk API. The response will be 
// passed to the ResponseReader. 
type Backend interface {
  Call(context.Context, *APIRequest, ResponseReader)
}

// ResponseReader reads API responses.
type ResponseReader interface {
  Read(*APIResponse)
}

// Queryable can set query string parameters.
type Queryable interface {
  Add(url.Values)
}

Along with these interfaces we have some basic types that describe core API resources. These are

// APIResource describes common functionality for all API resources. These could 
// be types for users, organizations, domains, etc.
// APIResource implements the ResponseReader interface. This way, we can build 
// resources out of API responses.
type APIResource struct {}

// APIParams describes common functionality for parameters that can be included in 
// API requests. 
type APIParams struct {}

// APIRequest is a wrapper over an http.Request for the Clerk API.
type APIRequest struct {}

// APIResponse is a wrapper over an http.Response for the Clerk API.
type APIResponse struct {}

// APIErrorResponse is a wrapper over an unsuccessful http.Response for the Clerk API.
// The Clerk API can return error responses, so the APIErrorResponse also implements the 
// Error interface.
type APIErrorResponse struct {}

A default Backend implementation is provided but not exported.

Using the above types, implementing an API operation should be easy. Take a look at the tests (clerk_test.go) for a few examples of how we would write API operations.

package domain

func Create(ctx context.Context, params *clerk.DomainParams) (*clerk.Domain, error) {
  req := clerk.NewAPIRequest("POST", "/domains")
  req.SetBody(params)
  dmn := &clerk.Domain{}
  err := clerk.GetBackend().Call(ctx, req, dmn)
  return dmn, err
}

Finally, consuming the API operation that we implemented above would look like this

clerk.SecretKey = "sk_live_123"
dmn, err := domain.Create(ctx, &clerk.DomainParams{})

As you might have guessed, the API and implementation is based on Stripe's Go SDK

[georgepsarakis]

❓ I don't think the secret key should be exported. Perhaps now is a good opportunity to move away from this since it is a breaking change?

[gkats]

@georgepsarakis Can you think of a problem with exporting the SecretKey on the package level?

The benefit is that you can set your key with a simple API.

clerk.SecretKey = "sk_live_123"

[gkats]

Made the variable non-exported and we now offer a clerk.SetKey("sk_live_123") method to configure your API key.

[georgepsarakis]

❓ Perhaps:

Suggested change

	var httpClient = &http.Client{
	var defaultHTTPClient = &http.Client{

[gkats]

Renamed. 😄

[georgepsarakis]

❓ I'm not sure I understand the use of these globals, along with the Get/SetBackend functions. I would maybe expect that a NewAPIClient method (if I understand the parity with the Backend interface here), returns a fully isolated client instance. Any chance we need to simplify some aspects here?

[gkats]

It depends.

Having a "singleton" global API client allows for two things:

1. Simple API for operations

clerk.SecretKey = "sk_live_123"
dmn, err := domain.Create(ctx, &DomainParams{})

2. Ability to stub out the client for tests

clerk.SetBackend(clerk.NewBackend(&clerk.BackendConfig{
  HTTPClient: myCustomClient,
}))

domain.Create(ctx, &clerk.DomainParams{})

I'm facing the same dilemma as you. Is the global singleton-like backend worth the trouble? Will it actually prove flexible or not? I toyed around with different approaches, but DX-wise, I believe nothing beats the API described above.

How would the alternative look like?

domain.NewClient(&clerk.BackendConfig{}).Create(ctx, &clerk.DomainParams{})

Or I imagine in your main.go file

package main

type Server struct {
  domainClient *domain.Client
  organizationClient *organization.Client
}

func main() {
  // Instantiate your clients and your server struct to ListenAndServe
  clerkConfig := &clerk.BackendConfig{
    HTTPClient: myCustomClient,
  }
  srv := &Server{
    domainClient: domain.NewClient(clerkConfig),
    organizationClient: organization.NewClient(clerkConfig),
  }
  // routes, handlers, and ListenAndServe
  srv.Start()
}

// and then in your handler
func (srv *Server) CreateDomains(w, r) {
  dmn, err := srv.domainClient.Create(ctx, &clerk.DomainParams{})
}

Comparing this solution, to the dmn, err := domain.Create() example above, there's a clear DX benefit in my eyes.

What do you think?

[georgepsarakis]

Ability to stub out the client for tests

The use of a global variable here seems to be breaking test isolation, if tests are running in parallel. So if I understand correctly we may not allow dependency injection 🤔 ?

I think you can avoid the (broader) use of globals with defining an explicit setter for a default backend and provide constructors for dependency injection. I imagine the API being something like:

package clerk

type Backend struct {
  httpClient *http.Client
}

var defaultBackend = &Backend{}
func SetDefaultBackend(cfg *BackendConfig) {
  // Resembles `SetBackend` now but mutates an existing struct
  defaultBackend.httpClient = cfg.HTTPClient
}

func DefaultBackend() *Backend {
  return defaultBackend
}

type DomainsClient struct {
  backend *Backend
}

func NewDomainsClient(backend *Backend) *DomainsClient {}

var Domains = struct {
  backend *Backend
}{backend: defaultBackend}

func (d *Domains) Create(ctx context.Context, params *CreateDomainParams) (Domain, error) {}

On the user's side:

package main

func main() {
  // or this can happen on the user's package-level init()
  clerk.SetDefaultBackend(...)
}

func CreateDomains(ctx context.Context, name string) (..., error) {
  domain, err := clerk.Domains.Create(ctx, &clerk.DomainParams{Name: name})
}

Just an idea, I'm not sure what the exact end-user API needs to be.

[gkats]

Resolved the discussion offline. We'll go ahead with the current design until there's more strong evidence towards change.

[georgepsarakis]

👏 Approving since this is foundational work in order to move things forward! There seems to be a failure for Go 1.17 that might be worth looking into.

[georgepsarakis]

❓ Just a note that you may want to retain the CI required check name. If I understand correctly all CI checks are optional now.

[alex-ntousias]

I really ❤️ where this is going! Great work @gkats .
@georgepsarakis raised some very valid concerns/questions. Other than that, looks great to me!

[alex-ntousias]

I believe this is not needed anymore, given our plan for API versioning.

[gkats]

👍 When we're done with API versioning I'll revisit. Leaving this mostly as a placeholder for now.

[alex-ntousias]

I know it's the Go convention, but APIURL looks sooo strange!