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

[React 19] useSyncExternalStore shim might break #29854

Closed
phryneas opened this issue Jun 11, 2024 · 8 comments · Fixed by #29868
Closed

[React 19] useSyncExternalStore shim might break #29854

phryneas opened this issue Jun 11, 2024 · 8 comments · Fixed by #29868

Comments

@phryneas
Copy link
Contributor

Summary

I just wanted to bring this to your attention - the uSES shim was previously packaged in a way that accessed __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED and the new 1.4 RC versions directly access __CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE.

So either way, there will be versions of the uSES shim and React that will break when compiling them with each other.

Are there any planned workarounds or things we should know as package authors that rely on the shim, that we should communicate to our consumers?
Should they pin their version?

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 12, 2024

This is a bug that's caused by our console.error transform. For anybody interested in contributing: We need to use an escape hatch and call console['error']() instead. For testing, you can check the build output in /build after yarn build use-sync-external-store and shouldn't find any mention of __CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE

@phryneas
Copy link
Contributor Author

Easy React PR? Hold my beer :)

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 12, 2024

It's not crashing, right? I think it just misses component stacks which console['error'] would also.

@phryneas
Copy link
Contributor Author

phryneas commented Jun 12, 2024

I'm pretty sure it wouldn't even compile because of the reference, and you've explicitly set that up to fail builds.

But I gotta be honest, I don't have the correct environment set up to actually test that assumption. I was reading the shipped source code when I stumbled upon this.

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 12, 2024

I'm pretty sure it wouldn't even compile because of the reference, and you've explicitly set that up to fail builds.

How did we set it up to fail builds?

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 12, 2024

Just double checked and we do have a test suite that runs against 17. It just doesn't test component stacks for warnings which would be missing when you use use-sync-external-store@rc and React 17.

@eps1lon
Copy link
Collaborator

eps1lon commented Jun 12, 2024

It does crash at runtime when we would trigger warnings. The only case right now is when you'd return an uncached snapshot:

const { text } = useSyncExternalStore(store.subscribe, () => {
  // Intentionally return an uncached snapshot to trigger warnings
  return { ...store.getState() };
});

-- https://codesandbox.io/p/sandbox/react-19-uses-old-react-vl547c

@phryneas
Copy link
Contributor Author

phryneas commented Jun 12, 2024

How did we set it up to fail builds?

From what I gather, by no longer exporting __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED from React , webpack will now crash when a module tries to import that property. I had assumed that was intentional.

At least we had reports of that happening. (As a stopgap, rehackt exports this as undefined if it doesn't exist in the upstream React version, and we're now migrating to a non-internals-accessing workaround)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants