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

[rcr] Re-export useMemoCache in top level React namespace #31050

Closed
wants to merge 10 commits into from

Conversation

poteto
Copy link
Member

@poteto poteto commented Sep 24, 2024

Stack from ghstack (oldest at bottom):

In order to support using the compiler on versions of React prior to 19,
we need the ability to statically import c (aka useMemoCache) or
fallback to a polyfill supplied by react-compiler-runtime (note: this
is a separate npm package, not to be confused with
react/compiler-runtime, which is currently a part of react).

To do this we first need to re-export useMemoCache under the top level
React namespace again, which is additive and thus non-breaking. Doing so
allows react-compiler-runtime to statically either re-export
React.__COMPILER_RUNTIME.c or supply a polyfill, without the need for
a dynamic import which is finicky to support due to returning a promise.

In later PRs I will remove react/compiler-runtime and update the
compiler to emit imports to react-compiler-runtime instead.

[ghstack-poisoned]
Copy link

vercel bot commented Sep 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 6:55pm

@react-sizebot
Copy link

react-sizebot commented Sep 24, 2024

The size diff is too large to display in a single comment. The GitHub action for this pull request contains an artifact called 'sizebot-message.md' with the full message.

Generated by 🚫 dangerJS against fc68cae

[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@josephsavona
Copy link
Contributor

To do this we first need to re-export useMemoCache under the top level React namespace again

Can you clarify this bit? I'm assuming it's so that we can do the feature detection, but why doesn't it work to check for react/compiler-runtime? It would just be really really nice to keep this out of the main public API.

@poteto
Copy link
Member Author

poteto commented Sep 27, 2024

for backwards compatibility we can't inject something like import * as ... from 'react/compiler-runtime' since that module wouldn't exist. we could use a dynamic import instead but that's not ergonomic for codegen since it returns a promise. and CJS wouldn't work either since it wouldn't play well with ESM-only bundlers.

if we export this from the main API (could also rename?) then it makes the import always work and then it's trivial to do the check and fallback

@josephsavona
Copy link
Contributor

Ahh that makes sense. I’m too used to require(). In that case, how about naming more generically and put this behind an object so that we could add other compiler runtime apis there later? Eg React.__compilerRuntume.c for the memo cache.

[ghstack-poisoned]
Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

Let's export an object so we have a single place to check for various runtime APIs we may add over time

[ghstack-poisoned]
@poteto poteto marked this pull request as ready for review October 1, 2024 18:21
[ghstack-poisoned]
@poteto
Copy link
Member Author

poteto commented Oct 1, 2024

Let's export an object so we have a single place to check for various runtime APIs we may add over time

Done!

[ghstack-poisoned]
[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants