Skip to content
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

feat: added async getSession/getUser method #285

Merged
merged 3 commits into from
Jun 4, 2022
Merged

Conversation

alaister
Copy link
Member

@alaister alaister commented May 17, 2022

What kind of change does this PR introduce?

This is a proof of concept for a new async getSession() method. This allows users to get a valid session (not expired) as opposed to the synchronous session() method, which may return an expired session.

Additional context

I know we're planning to discuss this further, but since I already had most of the code, I thought I'd throw something together quickly. It will hopefully make our discussion clearer if we have something to look at!

Once gotrue-js has a method like this (or similar) it will allow supabase-js to work something like:

async function getAccessToken() {
    const {session} = await auth.getSession()

    return session?.access_token ?? null
}

const postgrestClient = new PostgrestClient({ getAccessToken, ... })

Then whenever postgrestClient is about to send a query, it can call the getAccessToken() method, which will return an up-to-date token.

Note: While this isn't a breaking change in gotrue-js (purely additive), it will be a breaking change once we start doing the above in supabase-js, as some users may rely on the first request possible sending an expired token. We may consider ignoring this use case though, as it is a strange one!

Resolves #143 and #23
Supersedes #265 and #147

}

const hasExpired = this.currentSession.expires_at
? this.currentSession.expires_at <= Date.now() / 1000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have some threshold here? we already have a constate for this i think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed here? If this handler works for only refreshing the token if it's already somehow expired, and the regular refresh timer handles refreshing tokens early, users rarely have to wait for the refresh request before their normal request.

Not opposed to it, just wondering what the benefits are vs the drawbacks?

@alaister alaister changed the title POC: feat: added async getSession method [POC] feat: added async getSession method May 23, 2022
@alaister alaister changed the base branch from master to next May 25, 2022 14:31
@soedirgo
Copy link
Member

@alaister instead of getAccessToken in postgrest-js and storage-js, wdyt about using this approach instead?

// SupabaseClient constructor
this.rest = new PostgrestClient(url, {
  ...,
  fetch: async (input, init) => {
    const headers = init?.headers ?? {}
    const accessToken = await getAccessToken()
    if (accessToken) {
      headers['Authorization'] = `Bearer ${accessToken}`
    }
    return this.fetch(input, { ...init, headers })
  },
})

@alaister
Copy link
Member Author

@soedirgo That works too!

Did you see this: supabase/storage-js#67? It essentially makes all of the headers async. What are your thoughts on the benefits/drawbacks of all three approaches?

@soedirgo
Copy link
Member

soedirgo commented May 31, 2022

Ah yup, that works as well.

I think the only real drawback of .getAccessToken()/.getHeaders() is that it's kind of specific to supabase-js, and it feels weird for people using postgrest-js/storage-js directly that headers are set using a callback.

Meanwhile the .fetch() override kinda stretches the functionality of custom fetch - it's not really how it's meant to be used.

@alaister
Copy link
Member Author

I agree that a custom fetch is the most flexible, but you're right does feel a bit dirty.

Personally, I could go either way between the custom fetch and async getHeaders. One thing nice about getHeaders is we can expose it to users who want to get an access token some other way, and it feels like a first-party solution. That being said, we can just document how to use the custom fetch like that without adding any additional API surface area.

@inian would love your thoughts here too.

@alaister alaister changed the title [POC] feat: added async getSession method feat: added async getSession/getUser method Jun 1, 2022
@alaister alaister merged commit 9870fa5 into next Jun 4, 2022
@alaister alaister deleted the feat/async-getSession branch June 4, 2022 01:05
@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2022

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants