search

clerk / javascript

Official JavaScript repository for Clerk authentication
https://clerk.com
MIT License
1.18k stars 266 forks source link

getToken method returns null right after isSIgnedIn becomes true #2266

Closed kylegwalsh closed 6 months ago

kylegwalsh commented 11 months ago

Preliminary Checks

Reproduction / Replay Link

https://jam.dev/c/3bce6734-c313-4b82-a638-4022254bfb7a

Publishable key

pk_test_Y2xlcmsuY3Jpc3Auc2Vhc25haWwtOTEubGNsLmRldiQ

Description

Steps to reproduce:

  1. Log in using the useSignIn() hook
  2. Enable queries and call getToken() from the useAuth() hook once the user logs in (key off of the isSignedIn boolean)

Expected behavior:

I would expect getToken() to return the token the moment clerk indicates that isSignedIn is true. This is how the library behaved in version 0.10.6 of clerk-expo (and it's related clerk-react version).

Actual behavior:

getToken() returns null right after the user is signed in (a race condition). This causes a cascade of undesired behaviors in my particular app because I expect token to exist right after clerk informs me that the user isSignedIn.

Environment

System:
    OS: macOS 13.3.1
    CPU: (8) arm64 Apple M1
    Memory: 434.08 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node
    Yarn: 3.2.2 - /opt/homebrew/bin/yarn
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.0/bin/npm
    pnpm: 8.7.4 - /opt/homebrew/bin/pnpm
  Browsers:
    Chrome: 119.0.6045.199
    Safari: 16.4
  npmPackages:
    @babel/core: ^7.20.0 => 7.22.1 
    @babel/plugin-proposal-export-namespace-from: 7.18.9 => 7.18.9 
    @clerk/clerk-expo: 0.19.22 => 0.19.22 
    @expo/vector-icons: ^13.0.0 => 13.0.0 
    @floating-ui/react-native: 0.8.2 => 0.8.2 
    @formatjs/intl-locale: 2.4.41 => 2.4.41 
    @formatjs/intl-numberformat: 7.3.0 => 7.3.0 
    @formatjs/intl-pluralrules: 4.2.0 => 4.2.0 
    @gorhom/bottom-sheet: 4.4.5 => 4.4.5 
    @hlp/analytics: 1.0.0 => 1.0.0 
    @hlp/assets: 1.0.0 => 1.0.0 
    @hlp/backend: 1.0.0 => 1.0.0 
    @hlp/components: 1.0.0 => 1.0.0 
    @hlp/config: 1.0.0 => 1.0.0 
    @hlp/core: 1.0.0 => 1.0.0 
    @hlp/expo-plugins: 1.0.0 => 1.0.0 
    @hlp/hooks: 1.0.0 => 1.0.0 
    @hlp/theme: 1.0.0 => 1.0.0 
    @hookform/resolvers: 2.9.7 => 2.9.7 
    @react-native-aria/checkbox: 0.2.3 => 0.2.3 
    @react-native-async-storage/async-storage: 1.17.11 => 1.17.11 
    @react-native-community/netinfo: 9.3.7 => 9.3.7 
    @react-navigation/bottom-tabs: 6.3.2 => 6.3.2 
    @react-navigation/devtools: 6.0.8 => 6.0.8 
    @react-navigation/drawer: 6.5.0 => 6.5.0 
    @react-navigation/native: 6.0.8 => 6.0.8 
    @react-navigation/native-stack: 6.5.0 => 6.5.0 
    @react-navigation/stack: 6.2.2 => 6.2.2 
    @react-stately/checkbox: 3.3.1 => 3.3.1 
    @rnmapbox/maps: 10.0.0-beta.58 => 10.0.0-beta.58 
    @segment/analytics-react-native: 2.7.1 => 2.7.1 
    @segment/sovran-react-native: 0.4.4 => 0.4.4 
    @sentry/react-native: 4.13.0 => 4.13.0 
    @shopify/flash-list: 1.4.0 => 1.4.0 
    @shopify/react-native-skia: 0.1.190 => 0.1.190 
    @tanstack/react-query: 4.2.3 => 4.2.3 
    @trpc/client: 10.0.0-proxy-beta.19 => 10.0.0-proxy-beta.19 
    @trpc/react: 10.0.0-proxy-beta.19 => 10.0.0-proxy-beta.19 
    @types/aws-lambda: 8.10.101 => 8.10.101 
    @types/chroma-js: 2 => 2.4.0 
    @types/number-to-words: 1 => 1.2.1 
    @types/numeral: ^2 => 2.0.5 
    @types/react-helmet: 6 => 6.1.6 
    @types/react-native-typing-animation: 0.1.0 => 0.1.0 
    asyncstorage-down: 4.2.0 => 4.2.0 
    babel-plugin-module-resolver: 4.1.0 => 4.1.0 
    chroma-js: 2.4.2 => 2.4.2 
    date-fns: 2.29.3 => 2.29.3 
    eslint: 8.27.0 => 8.27.0 
    expo: ~48.0.21 => 48.0.21 
    expo-application: ~5.1.1 => 5.1.1 
    expo-asset: ~8.9.1 => 8.9.2 
    expo-auth-session: ~4.0.3 => 4.0.3 
    expo-av: ~13.2.1 => 13.2.1 
    expo-build-properties: ~0.6.0 => 0.6.0 
    expo-constants: ~14.2.1 => 14.2.1 
    expo-dev-client: ~2.2.1 => 2.2.1 
    expo-device: ~5.2.1 => 5.2.1 
    expo-document-picker: ~11.2.2 => 11.2.2 
    expo-file-system: ~15.2.2 => 15.2.2 
    expo-linear-gradient: ~12.1.2 => 12.1.2 
    expo-linking: ~4.0.1 => 4.0.1 
    expo-local-authentication: ~13.3.0 => 13.3.0 
    expo-localization: ~14.1.1 => 14.1.1 
    expo-location: ~15.1.1 => 15.1.1 
    expo-modules-core: ~1.2.7 => 1.2.7 
    expo-navigation-bar: ~2.1.1 => 2.1.1 
    expo-notifications: ~0.18.1 => 0.18.1 
    expo-router: 0.0.36 => 0.0.36 
    expo-secure-store: ~12.1.1 => 12.1.1 
    expo-sharing: ~11.2.2 => 11.2.2 
    expo-splash-screen: ~0.18.2 => 0.18.2 
    expo-status-bar: ~1.4.4 => 1.4.4 
    expo-store-review: ~6.2.1 => 6.2.1 
    expo-system-ui: ~2.2.1 => 2.2.1 
    expo-updates: ~0.16.4 => 0.16.4 
    expo-web-browser: ~12.1.1 => 12.1.1 
    expo-yarn-workspaces: 2.0.2 => 2.0.2 
    lottie-react-native: 5.1.4 => 5.1.4 
    moti: 0.20.0 => 0.20.0 
    native-base: v3.4.28 => 3.4.28 
    node-libs-react-native: 1.2.1 => 1.2.1 
    number-to-words: 1.2.4 => 1.2.4 
    numeral: 2.0.6 => 2.0.6 
    posthog-react-native: 2.1.0 => 2.1.0 
    react: 18.2.0 => 18.1.0 
    react-date-picker: 9.0.0 => 9.0.0 
    react-dom: 18.2.0 => 18.1.0 
    react-error-overlay: 6.0.9 => 6.0.9 
    react-helmet: 6.1.0 => 6.1.0 
    react-map-gl: 7.0.19 => 7.0.19 
    react-native: 0.71.14 => 0.71.14 
    react-native-appsflyer: 6.12.2 => 6.12.2 
    react-native-awesome-alerts: 2.0.0 => 2.0.0 
    react-native-bundle-visualizer: 3.1.1 => 3.1.1 
    react-native-confetti-cannon: 1.5.2 => 1.5.2 
    react-native-date-picker: 4.2.5 => 4.2.5 
    react-native-email-link: 1.14.3 => 1.14.3 
    react-native-fast-image: 8.5.11 => 8.5.11 
    react-native-gesture-handler: ~2.9.0 => 2.9.0 
    react-native-get-random-values: ~1.8.0 => 1.8.0 
    react-native-gifted-charts: 1.3.12 => 1.3.12 
    react-native-gifted-chat: 1.1.1 => 1.1.1 
    react-native-hyperlink: 0.0.22 => 0.0.22 
    react-native-linear-gradient: 2.8.3 => 2.8.3 
    react-native-lottie: 0.0.1-alpha.2 => 0.0.1-alpha.2 
    react-native-mask-input: 1.2.2 => 1.2.2 
    react-native-mmkv: 2.8.0 => 2.8.0 
    react-native-monorepo-tools: 1.2.0 => 1.2.0 
    react-native-plaid-link-sdk: 10.3.0 => 10.3.0 
    react-native-popper: 0.3.2 => 0.3.2 
    react-native-reanimated: 3.3.0 => 3.3.0 
    react-native-redash: 18.1.0 => 18.1.0 
    react-native-safe-area-context: 4.5.0 => 4.5.0 
    react-native-screens: ~3.20.0 => 3.20.0 
    react-native-svg: 13.14.0 => 13.14.0 
    react-native-swiper-flatlist: 3.0.17 => 3.0.17 
    react-native-toast-message: 2.1.5 => 2.1.5 
    react-native-typing-animation: 0.1.7 => 0.1.7 
    react-native-web: ~0.18.11 => 0.18.12 
    react-native-webview: 13.2.2 => 13.2.2 
    react-plaid-link: 3.4.0 => 3.4.0 
    react-select: 5.6.0 => 5.6.0 
    recharts: 2.9.0 => 2.9.0 
    rn-nodeify: 10.3.0 => 10.3.0 
    sentry-expo: ~6.1.0 => 6.1.1 
    smartlook-react-native-wrapper: 1.2.8 => 1.2.8 
    superjson: 1.9.1 => 1.9.1 
    ts-node: 10.9.1 => 10.9.1 
    zod: 3.20.2 => 3.20.2
desiprisg commented 8 months ago

@kylegwalsh , Having seen the video and checked the behaviour myself to double check, getToken is returning a Promise, and thus it will have to be called after isSignedIn has returned true. As seen below, getToken() returned null before the log for isSignedIn is printed and so, it makes sense that it might returnnull, if isSignedIn is not checked beforehand. I should also mention here, that one should not look at the sign-ins request in the network tab, but the touch request right after, as that is what is actually setting the session as the active one. image

Since you mentioned that you actually check the value of isSignedIn before you actually call getToken() (although the logs in the console seem to be reversed), could you provide a snippet of the code ?

kylegwalsh commented 8 months ago

Hi @desiprisg, my code uses @trpc/react to control queries to the backend. My query should not be enabled until isSignedIn becomes true (see below):

useUser() hook

  // Get the user data from our DB
  const { data } = trpc.user.get.useQuery(undefined, {
    // Don't enable the query until the clerk user is defined
    enabled: !!isSignedIn,
  });

QueryProvider component

import { useAuth } from '@clerk/clerk-expo';
import { config } from '@hlp/config';
import { trpc } from '@hlp/core/client';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { httpBatchLink } from '@trpc/client/links/httpBatchLink';
import React, { useState } from 'react';
import superjson from 'superjson';

/** Adds TRPC and react-query functionality to the app */
export const QueryProvider = ({ children }: any) => {
  const { getToken } = useAuth();

  // Initialize query client and trpc client
  const [queryClient] = useState(() => new QueryClient());
  const [trpcClient] = useState(() =>
    trpc.createClient({
      links: [
        httpBatchLink({
          url: `${config.api.url}/trpc/`,
          async headers() {
            /** Retrieve auth token from storage */
            const token = await getToken({
              template: 'default',
            });

            console.log('CLERK TOKEN', token);

            return {
              // Attach auth header if we have a token
              ...(token && {
                Authorization: `Bearer ${token}`,
              }),
            };
          },
        }),
      ],
      transformer: superjson,
    })
  );

  return (
    <trpc.Provider client={trpcClient} queryClient={queryClient}>
      <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
    </trpc.Provider>
  );
};

So, the useUser() query is being enabled by isSignedIn first, and then it is invoking getToken() during the request. This is why I thought that there may be some kind of race condition between the two actions.

I just attempted to upgrade to the latest clerk version to see if the issues still persist, but I'm seeing a CORS error due to an expo header (I never saw this before, so I'm not sure what introduced it). I'll have to dig into this more before I can determine if I'm still seeing the issue.

Access to fetch at 'https://CLERK_DOMAIN/v1/client?_clerk_js_version=4.70.5&_is_native=1&__dev_session=SESSION' from origin 'http://localhost:8081' has been blocked by CORS policy: Request header field x-expo-native-application-version is not allowed by Access-Control-Allow-Headers in preflight response.
gkamer8 commented 7 months ago

We also have pages protected by whose useEffects are triggered while await getToken() returns null (same situation as above - directly after the user signs in and gets redirected). Is there active work on this issue / what is the best practice here to prevent a double fetch?

thiskevinwang commented 6 months ago

[!NOTE] Those header(s) are coming from the Expo SDK. https://github.com/clerk/javascript/blob/69df1ab81b95ca2af82bf17bee9d3fa1a9fdbfa2/packages/expo/src/singleton.ts#L47-L53

We can remove these, and publish a new version.

Additionally, the CORS suggests that you're using Clerk-expo from a browser, is that right @kylegwalsh? Can you help us understand your development flow a bit? Clerk-expo via the browser is not exactly a supported use case yet, though we do have Support for Expo Web on our roadmap.

kylegwalsh commented 6 months ago

@thiskevinwang That would make sense. We've actually been using clerk-expo since the beginning for mobile / web and never encountered any issues (until we tried to upgrade). The error definitely makes sense if the latest version of clerk-expo doesn't support web.

Happy to expand on our development flow as well. We actually created a single expo codebase that works on every platform (ios, android, and web). Most of the libraries that we're using support that setup, however we do have to occasionally create two separate files (index.web.ts vs. index.native.ts) to import separate packages based on platform. I can definitely modify the setup to import clerk-react on web if that should work.

thiskevinwang commented 6 months ago

@kylegwalsh I've opened a PR to remove these headers - #3326

While that is in-review, you can also try clerk.__unstable__onBeforeRequest as a workaround to remove the headers before the request

thiskevinwang commented 6 months ago

Hey @kylegwalsh this should be fixed in @clerk/clerk-expo@1.1.0!

Give that a shot, and feel free to reopen this issue if the error persists!

npm install @clerk/clerk-expo@latest