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

Internalize useSyncExternalStore shim, for more control than use-sync-external-store provides #9675

Merged
merged 11 commits into from
May 9, 2022

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented May 4, 2022

This internalization solves several problems with the official use-sync-external-store/shim package:

  • We can ensure useLayoutEffect is called only on the client, eliminating noisy warnings
  • We can use our own internal mechanisms like __DEV__ to gate development-only code
  • We don't have to use an Object.is polyfill because we know our snapshots won't need it
  • Because the useSyncExternalStore shim is now implemented/exported using ECMAScript module syntax, issues like Vite + react + useQuery = use-sync-external-store/shim problem #9670 should be resolved

If there was some way to selectively import the different client/server versions of the useSyncExternalStore hook from use-sync-external-store/shim, I would be happy to keep using that package, but the difficulty of making the official package work exceeded the difficulty of implementing our own shim (this PR), and this approach gives us more control over the shimming.

I believe this is a more precise change than my previous commit, which
used isNode to determine canUseDOM. In most server-side rendering
setups, there will be no window.document.createElement, so canUseDOM
will already be false for that reason. It's only in Node.js environments
using jsdom that window.document.createElement might seem to be defined,
so that's the only case we really need to check.
@benjamn benjamn force-pushed the internalize-useSyncExternalStore-shim branch from 3641006 to 94b408a Compare May 9, 2022 20:00
@benjamn benjamn changed the base branch from release-3.7 to main May 9, 2022 20:00
This should mean we'll find out if anything changes in the types of
@types/use-sync-external-store that conflicts with how we use our
wrapper of the useSyncExternalStore hook.
Copy link
Contributor

@brainkim brainkim left a comment

Choose a reason for hiding this comment

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

I hope this rock kills a lot of birds.

@benjamn
Copy link
Member Author

benjamn commented May 9, 2022

I'm going to merge this to main because I'd like to release it in 3.6.x if we can verify it solves the problem (#9668, #9670, and others). However, I will merge main into release-3.7 first and publish a beta release from there, so we can test these changes in @apollo/client@3.7.0-alpha.x before publishing the 3.6.4 version.

@benjamn benjamn merged commit 1b9261a into main May 9, 2022
@benjamn benjamn deleted the internalize-useSyncExternalStore-shim branch May 9, 2022 21:45
@SimenB
Copy link
Contributor

SimenB commented May 10, 2022

@benjamn I get the following error with the alpha using webpack@5.72.0:

➤ YN0000: [bank-frontend]: ERROR in ../../node_modules/@apollo/client/react/hooks/useSyncExternalStore.js 6:19-45
➤ YN0000: [bank-frontend]: export 'useSyncExternalStore' (imported as 'React') was not found in 'react' (possible exports: Children, Component, Fragment, Profiler, PureComponent, StrictMode, Suspense, __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, cloneElement, createContext, createElement, createFactory, createRef, forwardRef, isValidElement, lazy, memo, useCallback, useContext, useDebugValue, useEffect, useImperativeHandle, useLayoutEffect, useMemo, useReducer, useRef, useState, version)
➤ YN0000: [bank-frontend]:  @ ../../node_modules/@apollo/client/react/hooks/useQuery.js 4:0-65 69:21-41
➤ YN0000: [bank-frontend]:  @ ../../node_modules/@apollo/client/react/hooks/index.js
➤ YN0000: [bank-frontend]:  @ ../../node_modules/@apollo/client/react/index.js 3:0-33 3:0-33
➤ YN0000: [bank-frontend]:  @ ../../node_modules/@apollo/client/index.js 2:0-33 2:0-33
➤ YN0000: [bank-frontend]:  @ ./src/index.tsx 4:0-48 48:32-46

(no error in 3.6.3 fwiw)

@benjamn
Copy link
Member Author

benjamn commented May 10, 2022

@SimenB Thanks! I think I can tweak the code so the expression React.useSyncExternalStore is never used directly, which should prevent that error when using React 17, but I wasn't immediately able to reproduce the problem in our create-react-app-based reproduction app. I updated to @apollo/client@beta and webpack@latest, but npm start seems to compile successfully. Any thoughts?

@SimenB
Copy link
Contributor

SimenB commented May 11, 2022

@benjamn make sure to also downgrade to react@17

image

diff --git i/package-lock.json w/package-lock.json
index e8b0943..d5fb5fe 100644
--- i/package-lock.json
+++ w/package-lock.json
@@ -9,10 +9,10 @@
       "version": "1.0.0",
       "license": "MIT",
       "dependencies": {
-        "@apollo/client": "^3.6.0",
+        "@apollo/client": "^3.7.0-alpha.3",
         "graphql": "^16.4.0",
-        "react": "^18.0.0",
-        "react-dom": "^18.0.0"
+        "react": "^17.0.0",
+        "react-dom": "^17.0.0"
       },
       "devDependencies": {
         "customize-cra": "^1.0.0",
@@ -23,11 +23,12 @@
       }
     },
     "node_modules/@apollo/client": {
-      "version": "3.6.0",
-      "resolved": "https://registry.npmjs.org/@apollo/client/-/client-3.6.0.tgz",
-      "integrity": "sha512-2+UZoe6fCI3qG9lz8Kdt5ranEllRk8ewpt3ma6fWMjbpt7AP9sprnnoG7n6UmBcOpUTOvAAcaxBf8nByRcXn7g==",
+      "version": "3.7.0-alpha.3",
+      "resolved": "https://registry.npmjs.org/@apollo/client/-/client-3.7.0-alpha.3.tgz",
+      "integrity": "sha512-jOUCnuDZYXFMkSaNo5q7+5ZgTkWvKNaxoohZPY+pBEFC8vDNUOCXN5koPpZmvUjl7kPsYI+V6TqR3ZWcefisUA==",
       "dependencies": {
         "@graphql-typed-document-node/core": "^3.1.1",
+        "@types/use-sync-external-store": "^0.0.3",
         "@wry/context": "^0.6.0",
         "@wry/equality": "^0.5.0",
         "@wry/trie": "^0.3.0",
@@ -36,9 +37,8 @@
         "optimism": "^0.16.1",
         "prop-types": "^15.7.2",
         "symbol-observable": "^4.0.0",
-        "ts-invariant": "^0.10.0",
+        "ts-invariant": "^0.10.2",
         "tslib": "^2.3.0",
-        "use-sync-external-store": "^1.0.0",
         "zen-observable-ts": "^1.2.0"
       },
       "peerDependencies": {
@@ -64,14 +64,6 @@
       "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.3.0.tgz",
       "integrity": "sha512-N82ooyxVNm6h1riLCoyS9e3fuJ3AMG2zIZs2Gd1ATcSFjSA23Q0fzjjZeh0jbJvWVDZ0cJT8yaNNaaXHzueNjg=="
     },
-    "node_modules/@apollo/client/node_modules/use-sync-external-store": {
-      "version": "1.0.0",
-      "resolved": "https://registry.npmjs.org/use-sync-external-store/-/use-sync-external-store-1.0.0.tgz",
-      "integrity": "sha512-AFVsxg5GkFg8GDcxnl+Z0lMAz9rE8DGJCc28qnBuQF7lac57B5smLcT37aXpXIIPz75rW4g3eXHPjhHwdGskOw==",
-      "peerDependencies": {
-        "react": "^16.8.0 || ^17.0.0 || ^18.0.0-rc"
-      }
-    },
     "node_modules/@babel/code-frame": {
       "version": "7.16.7",
       "resolved": "https://registry.npmjs.org/@babel/code-frame/-/code-frame-7.16.7.tgz",
@@ -3621,6 +3613,11 @@
       "integrity": "sha512-F5DIZ36YVLE+PN+Zwws4kJogq47hNgX3Nx6WyDJ3kcplxyke3XIzB8uK5n/Lpm1HBsbGzd6nmGehL8cPekP+Tg==",
       "dev": true
     },
+    "node_modules/@types/use-sync-external-store": {
+      "version": "0.0.3",
+      "resolved": "https://registry.npmjs.org/@types/use-sync-external-store/-/use-sync-external-store-0.0.3.tgz",
+      "integrity": "sha512-EwmlvuaxPNej9+T4v5AuBPJa2x2UOJVdjCtDHgcDqitUeOtjnJKJ+apYjVcAoBEMjKW1VVFGZLUb5+qqa09XFA=="
+    },
     "node_modules/@types/ws": {
       "version": "8.2.2",
       "resolved": "https://registry.npmjs.org/@types/ws/-/ws-8.2.2.tgz",
@@ -14079,11 +14076,12 @@
       }
     },
     "node_modules/react": {
-      "version": "18.0.0",
-      "resolved": "https://registry.npmjs.org/react/-/react-18.0.0.tgz",
-      "integrity": "sha512-x+VL6wbT4JRVPm7EGxXhZ8w8LTROaxPXOqhlGyVSrv0sB1jkyFGgXxJ8LVoPRLvPR6/CIZGFmfzqUa2NYeMr2A==",
+      "version": "17.0.2",
+      "resolved": "https://registry.npmjs.org/react/-/react-17.0.2.tgz",
+      "integrity": "sha512-gnhPt75i/dq/z3/6q/0asP78D0u592D5L1pd7M8P+dck6Fu/jJeL6iVVK23fptSUZj8Vjf++7wXA8UNclGQcbA==",
       "dependencies": {
-        "loose-envify": "^1.1.0"
+        "loose-envify": "^1.1.0",
+        "object-assign": "^4.1.1"
       },
       "engines": {
         "node": ">=0.10.0"
@@ -14274,15 +14272,16 @@
       }
     },
     "node_modules/react-dom": {
-      "version": "18.0.0",
-      "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.0.0.tgz",
-      "integrity": "sha512-XqX7uzmFo0pUceWFCt7Gff6IyIMzFUn7QMZrbrQfGxtaxXZIcGQzoNpRLE3fQLnS4XzLLPMZX2T9TRcSrasicw==",
+      "version": "17.0.2",
+      "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-17.0.2.tgz",
+      "integrity": "sha512-s4h96KtLDUQlsENhMn1ar8t2bEa+q/YAtj8pPPdIjPDGBDIVNsrD9aXNWqspUe6AzKCIG0C1HZZLqLV7qpOBGA==",
       "dependencies": {
         "loose-envify": "^1.1.0",
-        "scheduler": "^0.21.0"
+        "object-assign": "^4.1.1",
+        "scheduler": "^0.20.2"
       },
       "peerDependencies": {
-        "react": "^18.0.0"
+        "react": "17.0.2"
       }
     },
     "node_modules/react-error-overlay": {
@@ -14983,11 +14982,12 @@
       }
     },
     "node_modules/scheduler": {
-      "version": "0.21.0",
-      "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.21.0.tgz",
-      "integrity": "sha512-1r87x5fz9MXqswA2ERLo0EbOAU74DpIUO090gIasYTqlVoJeMcl+Z1Rg7WHz+qtPujhS/hGIt9kxZOYBV3faRQ==",
+      "version": "0.20.2",
+      "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.20.2.tgz",
+      "integrity": "sha512-2eWfGgAqqWFGqtdMmcL5zCMK1U8KlXv8SQFGglL3CEtd0aDVDWgeF/YoCmvln55m5zSk3J/20hTaSBeSObsQDQ==",
       "dependencies": {
-        "loose-envify": "^1.1.0"
+        "loose-envify": "^1.1.0",
+        "object-assign": "^4.1.1"
       }
     },
     "node_modules/schema-utils": {
@@ -16228,9 +16228,9 @@
       "dev": true
     },
     "node_modules/ts-invariant": {
-      "version": "0.10.1",
-      "resolved": "https://registry.npmjs.org/ts-invariant/-/ts-invariant-0.10.1.tgz",
-      "integrity": "sha512-dOmY3naALBtNyK+nrVGzD8DVxSJ9OIHragItZ3XvxGORNAZL6uszgQYaD3PW+TPh2NWNsOpuQUxznuvTkmcdqw==",
+      "version": "0.10.2",
+      "resolved": "https://registry.npmjs.org/ts-invariant/-/ts-invariant-0.10.2.tgz",
+      "integrity": "sha512-5BybOL23OXYmmnA0C8NYPkUo5Kb/I4IVQk31K1VcdBZpQIn4fWKMIORGBJqGkwvDLyu9cxUb4Zv4G6xA4/07IQ==",
       "dependencies": {
         "tslib": "^2.1.0"
       },
@@ -16239,9 +16239,9 @@
       }
     },
     "node_modules/ts-invariant/node_modules/tslib": {
-      "version": "2.3.1",
-      "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.3.1.tgz",
-      "integrity": "sha512-77EbyPPpMz+FRFRuAFlWMtmgUWGe9UOG2Z25NqCwiIjRhOf5iKGuzSe5P2w1laq+FkRy4p+PCuVkJSGkzTEKVw=="
+      "version": "2.4.0",
+      "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.4.0.tgz",
+      "integrity": "sha512-d6xOpEDfsi2CZVlPQzGeux8XMwLT9hssAsaPYExaQMuYskwb+x1x7J371tWlbBdWHroy99KnVB6qIkUbs5X3UQ=="
     },
     "node_modules/tsconfig-paths": {
       "version": "3.14.1",
@@ -17603,11 +17603,12 @@
   },
   "dependencies": {
     "@apollo/client": {
-      "version": "3.6.0",
-      "resolved": "https://registry.npmjs.org/@apollo/client/-/client-3.6.0.tgz",
-      "integrity": "sha512-2+UZoe6fCI3qG9lz8Kdt5ranEllRk8ewpt3ma6fWMjbpt7AP9sprnnoG7n6UmBcOpUTOvAAcaxBf8nByRcXn7g==",
+      "version": "3.7.0-alpha.3",
+      "resolved": "https://registry.npmjs.org/@apollo/client/-/client-3.7.0-alpha.3.tgz",
+      "integrity": "sha512-jOUCnuDZYXFMkSaNo5q7+5ZgTkWvKNaxoohZPY+pBEFC8vDNUOCXN5koPpZmvUjl7kPsYI+V6TqR3ZWcefisUA==",
       "requires": {
         "@graphql-typed-document-node/core": "^3.1.1",
+        "@types/use-sync-external-store": "^0.0.3",
         "@wry/context": "^0.6.0",
         "@wry/equality": "^0.5.0",
         "@wry/trie": "^0.3.0",
@@ -17616,9 +17617,8 @@
         "optimism": "^0.16.1",
         "prop-types": "^15.7.2",
         "symbol-observable": "^4.0.0",
-        "ts-invariant": "^0.10.0",
+        "ts-invariant": "^0.10.2",
         "tslib": "^2.3.0",
-        "use-sync-external-store": "^1.0.0",
         "zen-observable-ts": "^1.2.0"
       },
       "dependencies": {
@@ -17626,12 +17626,6 @@
           "version": "2.3.0",
           "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.3.0.tgz",
           "integrity": "sha512-N82ooyxVNm6h1riLCoyS9e3fuJ3AMG2zIZs2Gd1ATcSFjSA23Q0fzjjZeh0jbJvWVDZ0cJT8yaNNaaXHzueNjg=="
-        },
-        "use-sync-external-store": {
-          "version": "1.0.0",
-          "resolved": "https://registry.npmjs.org/use-sync-external-store/-/use-sync-external-store-1.0.0.tgz",
-          "integrity": "sha512-AFVsxg5GkFg8GDcxnl+Z0lMAz9rE8DGJCc28qnBuQF7lac57B5smLcT37aXpXIIPz75rW4g3eXHPjhHwdGskOw==",
-          "requires": {}
         }
       }
     },
@@ -20197,6 +20191,11 @@
       "integrity": "sha512-F5DIZ36YVLE+PN+Zwws4kJogq47hNgX3Nx6WyDJ3kcplxyke3XIzB8uK5n/Lpm1HBsbGzd6nmGehL8cPekP+Tg==",
       "dev": true
     },
+    "@types/use-sync-external-store": {
+      "version": "0.0.3",
+      "resolved": "https://registry.npmjs.org/@types/use-sync-external-store/-/use-sync-external-store-0.0.3.tgz",
+      "integrity": "sha512-EwmlvuaxPNej9+T4v5AuBPJa2x2UOJVdjCtDHgcDqitUeOtjnJKJ+apYjVcAoBEMjKW1VVFGZLUb5+qqa09XFA=="
+    },
     "@types/ws": {
       "version": "8.2.2",
       "resolved": "https://registry.npmjs.org/@types/ws/-/ws-8.2.2.tgz",
@@ -27928,11 +27927,12 @@
       }
     },
     "react": {
-      "version": "18.0.0",
-      "resolved": "https://registry.npmjs.org/react/-/react-18.0.0.tgz",
-      "integrity": "sha512-x+VL6wbT4JRVPm7EGxXhZ8w8LTROaxPXOqhlGyVSrv0sB1jkyFGgXxJ8LVoPRLvPR6/CIZGFmfzqUa2NYeMr2A==",
+      "version": "17.0.2",
+      "resolved": "https://registry.npmjs.org/react/-/react-17.0.2.tgz",
+      "integrity": "sha512-gnhPt75i/dq/z3/6q/0asP78D0u592D5L1pd7M8P+dck6Fu/jJeL6iVVK23fptSUZj8Vjf++7wXA8UNclGQcbA==",
       "requires": {
-        "loose-envify": "^1.1.0"
+        "loose-envify": "^1.1.0",
+        "object-assign": "^4.1.1"
       }
     },
     "react-app-polyfill": {
@@ -28073,12 +28073,13 @@
       }
     },
     "react-dom": {
-      "version": "18.0.0",
-      "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-18.0.0.tgz",
-      "integrity": "sha512-XqX7uzmFo0pUceWFCt7Gff6IyIMzFUn7QMZrbrQfGxtaxXZIcGQzoNpRLE3fQLnS4XzLLPMZX2T9TRcSrasicw==",
+      "version": "17.0.2",
+      "resolved": "https://registry.npmjs.org/react-dom/-/react-dom-17.0.2.tgz",
+      "integrity": "sha512-s4h96KtLDUQlsENhMn1ar8t2bEa+q/YAtj8pPPdIjPDGBDIVNsrD9aXNWqspUe6AzKCIG0C1HZZLqLV7qpOBGA==",
       "requires": {
         "loose-envify": "^1.1.0",
-        "scheduler": "^0.21.0"
+        "object-assign": "^4.1.1",
+        "scheduler": "^0.20.2"
       }
     },
     "react-error-overlay": {
@@ -28587,11 +28588,12 @@
       }
     },
     "scheduler": {
-      "version": "0.21.0",
-      "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.21.0.tgz",
-      "integrity": "sha512-1r87x5fz9MXqswA2ERLo0EbOAU74DpIUO090gIasYTqlVoJeMcl+Z1Rg7WHz+qtPujhS/hGIt9kxZOYBV3faRQ==",
+      "version": "0.20.2",
+      "resolved": "https://registry.npmjs.org/scheduler/-/scheduler-0.20.2.tgz",
+      "integrity": "sha512-2eWfGgAqqWFGqtdMmcL5zCMK1U8KlXv8SQFGglL3CEtd0aDVDWgeF/YoCmvln55m5zSk3J/20hTaSBeSObsQDQ==",
       "requires": {
-        "loose-envify": "^1.1.0"
+        "loose-envify": "^1.1.0",
+        "object-assign": "^4.1.1"
       }
     },
     "schema-utils": {
@@ -29538,17 +29540,17 @@
       "dev": true
     },
     "ts-invariant": {
-      "version": "0.10.1",
-      "resolved": "https://registry.npmjs.org/ts-invariant/-/ts-invariant-0.10.1.tgz",
-      "integrity": "sha512-dOmY3naALBtNyK+nrVGzD8DVxSJ9OIHragItZ3XvxGORNAZL6uszgQYaD3PW+TPh2NWNsOpuQUxznuvTkmcdqw==",
+      "version": "0.10.2",
+      "resolved": "https://registry.npmjs.org/ts-invariant/-/ts-invariant-0.10.2.tgz",
+      "integrity": "sha512-5BybOL23OXYmmnA0C8NYPkUo5Kb/I4IVQk31K1VcdBZpQIn4fWKMIORGBJqGkwvDLyu9cxUb4Zv4G6xA4/07IQ==",
       "requires": {
         "tslib": "^2.1.0"
       },
       "dependencies": {
         "tslib": {
-          "version": "2.3.1",
-          "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.3.1.tgz",
-          "integrity": "sha512-77EbyPPpMz+FRFRuAFlWMtmgUWGe9UOG2Z25NqCwiIjRhOf5iKGuzSe5P2w1laq+FkRy4p+PCuVkJSGkzTEKVw=="
+          "version": "2.4.0",
+          "resolved": "https://registry.npmjs.org/tslib/-/tslib-2.4.0.tgz",
+          "integrity": "sha512-d6xOpEDfsi2CZVlPQzGeux8XMwLT9hssAsaPYExaQMuYskwb+x1x7J371tWlbBdWHroy99KnVB6qIkUbs5X3UQ=="
         }
       }
     },
diff --git i/package.json w/package.json
index 7fb81e4..a713e29 100644
--- i/package.json
+++ w/package.json
@@ -4,10 +4,10 @@
   "version": "1.0.0",
   "license": "MIT",
   "dependencies": {
-    "@apollo/client": "^3.6.0",
+    "@apollo/client": "^3.7.0-alpha.3",
     "graphql": "^16.4.0",
-    "react": "^18.0.0",
-    "react-dom": "^18.0.0"
+    "react": "^17.0.0",
+    "react-dom": "^17.0.0"
   },
   "devDependencies": {
     "customize-cra": "^1.0.0",

@benjamn
Copy link
Member Author

benjamn commented May 11, 2022

@SimenB Ah yes, thanks, that was the trick to reproducing it reliably. See #9709 for a solution!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite + react + useQuery = use-sync-external-store/shim problem
3 participants