Skip to content

feature): [gotrue-js] Refactor return and error types #301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jun 23, 2022
Merged

Conversation

dthyresson
Copy link
Contributor

What kind of change does this PR introduce?

This PR refactors the return types for the gotrue-js client to match the pattern used by storage-js.

See: supabase/storage-js#60

There are some type errors, and functions return everything with | null, making it difficult for end-users to work with.

What is the current behavior?

Gotrue-js types are hard to work with and would benefit from union types everywhere. See what we’ve done in storage-js for inspiration

See: supabase/storage-js#60

What is the new behavior?

  • Functions now return type unions, which are more correct and "easier to work with".
  • Updates tests
  • Adds new error types
  • Updates cross-fetch version to match storage

Additional context

Tests pass.

gotrue-js dt-error-types % npm run test:suite

> @supabase/[email protected] test:suite
> jest --runInBand

 PASS  test/GoTrueClient.test.ts
 PASS  test/GoTrueApi.test.ts
 PASS  test/lib/utils.test.ts

Test Suites: 3 passed, 3 total
Tests:       2 skipped, 63 passed, 65 total
Snapshots:   0 total
Time:        7.159 s, estimated 8 s
Ran all test suites.

@dthyresson dthyresson self-assigned this Jun 8, 2022
@dthyresson
Copy link
Contributor Author

@kangmingtay --

  • This is a Draft WIP PR where tests pass locally, but CI fails. Fixing.
  1. In some places I left the | null like
Promise<
    | {
        session: Session | null
        user: User | null
        provider?: Provider
        url?: string | null
        error: null
      }
    | {
        session: Session | null
        user: User | null
        provider?: Provider
        url?: string | null
        error: AuthError
      }
  >

But perhaps I should enumerate the possibilities (I did so when there were fewer nullable cases)?

  1. There are many repeated, common return types. Should I create types from these so less copy paste and easier to make changes moving forward?

For example:

Promise<
    | {
        data: User
        error: null
      }
    | {
        data: Session
        error: null
      }
    | { data: null; error: AuthError }
  >

I could make this a UserOrSession or DataAsUserOrSession something along those lines and reduce repeated code.

@dthyresson dthyresson requested a review from kangmingtay June 8, 2022 19:06
@dthyresson
Copy link
Contributor Author

Also what changes to the documentation need to be made? Any new error handling?

@kangmingtay kangmingtay requested a review from alaister June 9, 2022 01:31
@dthyresson
Copy link
Contributor Author

Also, should the release and package be updated to 2?

{
  "name": "@supabase/gotrue-js",
  "version": "0.2.0",

and RELEASES?

# Releases

Releases are handled by Semantic release. This document is for forcing and documenting any non-code changes.

### v1.27 

- Fix: https://github.com/supabase/gotrue-js/pull/184

### v1.12.7

@kangmingtay
Copy link
Member

  1. In some places I left the | null like
Promise<
    | {
        session: Session | null
        user: User | null
        provider?: Provider
        url?: string | null
        error: null
      }
    | {
        session: Session | null
        user: User | null
        provider?: Provider
        url?: string | null
        error: AuthError
      }
  >

But perhaps I should enumerate the possibilities (I did so when there were fewer nullable cases)?

Would it make sense to just have everything in the Session type since it also contains the User type? We could also add the Provider type inside Session so that there are fewer possibilities to consider?

@kangmingtay
Copy link
Member

  1. There are many repeated, common return types. Should I create types from these so less copy paste and easier to make changes moving forward?

For example:

Promise<
    | {
        data: User
        error: null
      }
    | {
        data: Session
        error: null
      }
    | { data: null; error: AuthError }
  >

I could make this a UserOrSession or DataAsUserOrSession something along those lines and reduce repeated code.

Looking at this, it makes sense to remove the type check for User since Session has a User type in it. Also, if the User is null, then i guess the Session would be null too?

@alaister also has some thoughts on why he prefers not to have custom types for these

@dthyresson
Copy link
Contributor Author

dthyresson commented Jun 9, 2022

Looking at this, it makes sense to remove the type check for User since Session has a User type in it. Also, if the User is null, then i guess the Session would be null too?

One of the more confusing aspects of this api is the use of user vs session vs data.

Sometimes data is a user, sometimes a session, sometimes it returns user and sometimes session.

Is this the time to refactor to be consistent and not use “data” when a user or session is returned?

For example:

      let session: Session | null = null
      let user: User | null = null

      if ((data as Session).access_token) {
        session = data as Session
        user = session.user as User
        this._saveSession(session)
        this._notifyAllSubscribers('SIGNED_IN')
      }

      if ((data as User).id) {
        user = data as User
      }

      return { user, session, error: null }

In a signUp when as a dev would I want the User and when the Session?

It looks like signUpWithPhone and signUpWithEmail both always return a session.

It strikes me that when authenticating, I'd get a Session. But when querying for some user account info, I'd get a User. No?

The signInWithEmail only returns a Session.

@alaister alaister changed the base branch from master to next June 14, 2022 00:39
@dthyresson dthyresson marked this pull request as ready for review June 15, 2022 13:44
@dthyresson dthyresson changed the title Draft: WIP - (feature): [gotrue-js] Refactor return and error types feature): [gotrue-js] Refactor return and error types Jun 15, 2022
@dthyresson dthyresson requested a review from kangmingtay June 15, 2022 13:53
@dthyresson
Copy link
Contributor Author

This PR is ready for review.

Should include latest from next branch.

I tried in most places to make the return types consistent -- so fewer cases of where "data" meant "User" or "data" meant "Session" and instead return user or session so the developer knows what type that the "data" really is.

Also, the feature to include the shouldThrowOnError config is being deferred to v2.0.1.

@kangmingtay kangmingtay added the v2 gotrue-js v2 (to be merged into `next` branch) label Jun 16, 2022
@kangmingtay kangmingtay merged commit cc42ed2 into next Jun 23, 2022
@kangmingtay kangmingtay deleted the dt-error-types branch June 23, 2022 01:50
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.23.0-next.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.0.0-rc.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @next released on @rc v2 gotrue-js v2 (to be merged into `next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants