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

Fix Expo tokenCache #75

Merged
merged 2 commits into from
Mar 4, 2022
Merged

Fix Expo tokenCache #75

merged 2 commits into from
Mar 4, 2022

Conversation

devchampian
Copy link
Contributor

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/types
  • @clerk/clerk-expo
  • @clerk/backend-core
  • @clerk/clerk-sdk-node
  • @clerk/edge
  • build/tooling/chore

Description

  • ⛔️ npm test runs as expected.
  • ⛔️ npm run build runs as expected.

☝️ Both are failing but unrelated to my changes. The build succeeds for clerk-expo package without errors.

This fix does two things.

The first is add a guard to check for the existence of tokenCache.getToken and tokenCache.saveToken. Previously, if a tokenCache was passed in, but had different methods on it, e.g. get / set, it would attempt to call undefined functions instead of falling back to the getTokenFromMemory and saveTokenInMemory operations, which are sensible defaults.

The second change here adds an early return if the tokenCache is not provided at all. This makes an assumption that the token cache is not available for the web platform, which is the case with expo-secure-store that is being used in our clerk-expo-starter.

Ideally, we should do better detection of the platform instead of coupling the logic with the tokenCache. One option would be to use Platform.OS === 'web' provided by react-native itself, but that introduces react-native as a package dependency.

This fix, coupled with changes I recently made to the clerk-expo-starter, allow the starter to render on the web without additional changes. It also means we could potentially remove or update this banner in the Docs:

Clerk Expo banner

Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

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

💯

@devchampian devchampian merged commit a1eb428 into main Mar 4, 2022
@devchampian devchampian deleted the fix/expo_token_cache branch March 4, 2022 13:44
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants