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

Client "piggybacking" does not work, which causes skipped re-renders on many sign-in operations #4372

Open
4 tasks done
tmoran-stenoa opened this issue Oct 19, 2024 · 0 comments
Labels
needs-triage A ticket that needs to be triaged by a team member

Comments

@tmoran-stenoa
Copy link
Contributor

Preliminary Checks

Reproduction

Publishable key

Description

I apologize for not providing a minimal reproduction, I am currently in a rush. To compensate, I did some deep debugging and have identified the cause of the bug, which I describe below.

Issue

In any client-side React runtime, signIn.attemptFirstFactor() does not cause a re-render of the useSignIn() hook, meaning that components who are subscribed to that hook do not get re-rendered. This also happens for various other signIn operations as well, but this is the one that sent me down a rabbit hole for hours.

The reason this happens is because of the following function, which is supposed to ultimately trigger the re-render but actually doesn't:

protected static _updateClient<J>(responseJSON: FapiResponseJSON<J> | null): void {
if (!responseJSON) {
return;
}
// TODO: Revise Client piggybacking
const client = responseJSON.client || responseJSON.meta?.client;
if (client && BaseResource.clerk) {
BaseResource.clerk.updateClient(Client.getInstance().fromJSON(client));
}
}

The issue is on line 88: BaseResource.clerk and Client.getInstance() resolve to the same object. Because of this, the same Client object gets mutated instead of a copy being created, and therefore, the React memoization logic fails.

The React memoization logic I'm referring to is the stateWithMemoizedResources function, which invokes clientChanged to determine if the client object has changed. Although the logic of clientChanged is correct, because of the above, the function actually always returns false, even if the client was updated. The reason this is happening is because prev and next are the same object.

function clientChanged(prev: ClientResource, next: ClientResource): boolean {
return (
prev.id !== next.id ||
prev.updatedAt!.getTime() < next.updatedAt!.getTime() ||
prev.sessions.length !== next.sessions.length
);
}

How to reproduce this issue

I will fill this in later if needed. However, the above should be easy to validate by checking that prev === next in every invocation of clientChanged.

Environment

System:
    OS: macOS 14.5
    CPU: (8) arm64 Apple M1
    Memory: 61.69 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.16.0 - ~/.nvm/versions/node/v20.16.0/bin/node
    npm: 10.9.0 - ~/.nvm/versions/node/v20.16.0/bin/npm
    Watchman: 2024.01.22.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 129.0.6668.101
    Safari: 17.5
  npmPackages:
    @babel/core: ^7.22.5 => 7.22.5 
    @babel/preset-typescript: ^7.22.5 => 7.22.5 
    @babel/runtime: ^7.22.5 => 7.22.5 
    @clerk/clerk-js: ^5.15.1 => 5.15.1 
    @clerk/clerk-react: ^5.4.2 => 5.4.2 
    @clerk/shared: ^2.5.2 => 2.5.2 
    @clerk/types: ^4.14.0 => 4.14.0 
    @fortawesome/fontawesome-svg-core: ^6.4.2 => 6.4.2 
    @fortawesome/free-brands-svg-icons: ^6.4.2 => 6.4.2 
    @fortawesome/free-regular-svg-icons: ^6.4.2 => 6.4.2 
    @fortawesome/free-solid-svg-icons: ^6.4.2 => 6.4.2 
    @fortawesome/react-native-fontawesome: ^0.3.0 => 0.3.0 
    @gorhom/bottom-sheet: ^4.6.3 => 4.6.3 
    @notifee/react-native: ^7.2.0 => 7.7.1 
    @react-native-async-storage/async-storage: ^1.18.2 => 1.18.2 
    @react-native-camera-roll/camera-roll: ^5.6.0 => 5.6.0 
    @react-native-community/blur: ^4.4.0 => 4.4.0 
    @react-native-community/datetimepicker: ^7.1.0 => 7.1.0 
    @react-native-community/netinfo: ^9.3.10 => 9.3.10 
    @react-native-community/slider: ^4.2.4 => 4.4.2 
    @react-native-firebase/app: ^18.0.0 => 18.0.0 
    @react-native-firebase/messaging: ^18.0.0 => 18.0.0 
    @react-native-menu/menu: ^0.9.1 => 0.9.1 
    @react-navigation/bottom-tabs: ^6.5.19 => 6.5.19 
    @react-navigation/elements: ^1.3.29 => 1.3.30 
    @react-navigation/material-top-tabs: ^6.6.12 => 6.6.12 
    @react-navigation/native: ^6.1.16 => 6.1.16 
    @react-navigation/native-stack: ^6.9.26 => 6.9.26 
    @total-typescript/ts-reset: ^0.6.1 => 0.6.1 
    @types/base-64: ^1.0.2 => 1.0.2 
    @types/dateformat: ^3.0.1 => 3.0.1 
    @types/jest: ^26.0.23 => 26.0.24 
    @types/mime-types: ^2.1.1 => 2.1.1 
    @types/react-native: ^0.65.0 => 0.65.31 
    @types/react-test-renderer: ^17.0.1 => 17.0.2 
    @typescript-eslint/eslint-plugin: ^7.0.2 => 7.0.2 
    @typescript-eslint/parser: ^7.0.2 => 7.0.2 
    @zxcvbn-ts/language-en: ^3.0.2 => 3.0.2 
    @zxcvbn-ts/language-fr: ^3.0.2 => 3.0.2 
    babel-jest: ^29.5.0 => 29.5.0 
    base-64: ^1.0.0 => 1.0.0 
    dateformat: ^5.0.3 => 5.0.3 
    eslint: ^8.56.0 => 8.56.0 
    eslint-config-prettier: ^9.1.0 => 9.1.0 
    eventemitter3: ^5.0.1 => 5.0.1 
    husky: ^8.0.3 => 8.0.3 
    i18next: ^23.1.0 => 23.1.0 
    jest: ^26.6.3 => 26.6.3 
    lint-staged: ^13.1.1 => 13.2.2 
    metro-react-native-babel-preset: ^0.76.6 => 0.76.6 
    mime-types: ^2.1.34 => 2.1.35 
    mixpanel-react-native: ^2.3.1 => 2.3.1 
    patch-package: ^7.0.2 => 7.0.2 
    prettier: ^2.8.8 => 2.8.8 
    react: ^18.2.0 => 18.2.0 
    react-dom: file:react-dom-polyfill => 18.3.1 
    react-error-boundary: ^4.0.10 => 4.0.10 
    react-i18next: ^13.0.0 => 13.0.0 
    react-native: ^0.71.11 => 0.71.11 
    react-native-bouncy-checkbox: ^3.0.7 => 3.0.7 
    react-native-config: ^1.5.1 => 1.5.1 
    react-native-device-info: ^10.6.0 => 10.6.0 
    react-native-document-picker: ^9.0.1 => 9.0.1 
    react-native-draggable-flatlist: ^4.0.1 => 4.0.1 
    react-native-encrypted-storage: ^4.0.3 => 4.0.3 
    react-native-fs: ^2.18.0 => 2.20.0 
    react-native-gesture-handler: ^2.15.0 => 2.15.0 
    react-native-haptic-feedback: ^2.2.0 => 2.2.0 
    react-native-image-crop-picker: ^0.41.2 => 0.41.2 
    react-native-image-picker: ^7.1.2 => 7.1.2 
    react-native-image-resizer: ^1.4.5 => 1.4.5 
    react-native-image-zoom-viewer: ^3.0.1 => 3.0.1 
    react-native-inappbrowser-reborn: ^3.7.0 => 3.7.0 
    react-native-keyboard-controller: ^1.13.3 => 1.13.3 
    react-native-mmkv: ^2.11.0 => 2.11.0 
    react-native-modal: ^13.0.1 => 13.0.1 
    react-native-modal-datetime-picker: ^15.0.1 => 15.0.1 
    react-native-orientation-locker: ^1.7.0 => 1.7.0 
    react-native-pager-view: ^6.2.0 => 6.2.0 
    react-native-permissions: ^3.10.0 => 3.10.0 
    react-native-promise-rejection-utils: ^0.0.1 => 0.0.1 
    react-native-reanimated: ^3.14.0 => 3.14.0 
    react-native-restart: ^0.0.27 => 0.0.27 
    react-native-safe-area-context: ^4.5.3 => 4.5.3 
    react-native-screens: ^3.29.0 => 3.29.0 
    react-native-share: ^8.2.2 => 8.2.2 
    react-native-svg: ^13.9.0 => 13.9.0 
    react-native-tab-view: ^3.1.1 => 3.5.1 
    react-native-ui-datepicker: ^2.0.3 => 2.0.3 
    react-native-video: ^5.2.1 => 5.2.1 
    react-native-vision-camera: ^3.6.4 => 3.6.4 
    react-native-watch-connectivity: ^1.0.11 => 1.1.0 
    stream-chat: ^8.9.0 => 8.9.0 
    stream-chat-react-native: ^5.15.1 => 5.15.1 
    stream-chat-react-native-core: ^5.15.1 => 5.15.1 
    swr: ^2.1.5 => 2.2.5 
    ts-custom-error: ^3.3.1 => 3.3.1 
    ts-retry: ^4.1.2 => 4.2.0 
    typescript: ^5.6.3 => 5.6.3 
    use-debounce: ^10.0.0 => 10.0.0
@tmoran-stenoa tmoran-stenoa added the needs-triage A ticket that needs to be triaged by a team member label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage A ticket that needs to be triaged by a team member
Projects
None yet
Development

No branches or pull requests

1 participant