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

[compiler] Allow known React modules to be extended #31138

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tyxla
Copy link

@tyxla tyxla commented Oct 7, 2024

Summary

This PR suggests allowing the known React modules list to be extended.

Currently, in Gutenberg (The Block Editor project for WordPress) we expose (through re-exporting) React and React DOM APIs through the @wordpress/element package. In order for the React Compiler to treat the imports correctly, we need to be able to add @wordpress/element to the list of known packages.

The purpose is to expose this through a knownReactModules environment config property.

How did you test this change?

I would still like to add some tests to verify this works as expected.

Copy link

vercel bot commented Oct 7, 2024

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

Name Status Preview Comments Updated (UTC)
react-compiler-playground ❌ Failed (Inspect) Oct 7, 2024 5:27pm

@mofeiZ
Copy link
Contributor

mofeiZ commented Oct 7, 2024

Thanks for the summary!

We've been evolving compiler flags as we try to optimize for perf internally. One recently added option Environment::moduleTypeProvider looks closer to what you need. This would be useful when / if @wordpress/element wants to supply 'type definitions' for its const / function / hook exports. The main long term issue with this PR's approach is that it wouldn't let @wordpress/element export its own type definitions or define overrides if you ever add non-trivial wrapper logic to react hooks.

We haven't yet added support for defining react-like hooks (such as your re-exports) in moduleTypeProviders. There's currently a few ways we give React hooks special treatment.

  1. The hook identity (hookKind)
  • useMemo and useCallback hooks get inlined and have their dependencies checked against inferred dependencies
  • useRef gets checked for .current access during render
  1. The nominally typed return.

@josephsavona @mvitousek Thoughts on expanding TypeSchema to include something like BuiltInUseState | BuiltInUseRef etc, vs allowing library authors to add additional annotations to functions?
Examples of annotations could be that return type is 'stable' | 'ref-like', or function has 'use-like' reactivity or 'set-state' side effects

@josephsavona
Copy link
Contributor

Hmm, @tyxla can you share more context on why @wordpress/element is re-exporting React APIs? We would generally expect that developers consuming React do so via our packages, and the compiler is optimized around that.

@tyxla
Copy link
Author

tyxla commented Oct 8, 2024

Thank you @mofeiZ and @josephsavona for the swift feedback!

@josephsavona @mvitousek Thoughts on expanding TypeSchema to include something like BuiltInUseState | BuiltInUseRef etc, vs allowing library authors to add additional annotations to functions? Examples of annotations could be that return type is 'stable' | 'ref-like', or function has 'use-like' reactivity or 'set-state' side effects

This could potentially work for us, for what it's worth. From my perspective, React Compiler should have a way to understand if a module is a re-export of an original React module? After all, module aliases aren't unseen across the ecosystem.

Hmm, @tyxla can you share more context on why @wordpress/element is re-exporting React APIs?

Sure thing. Gutenberg is the block editor for WordPress, which runs on a major part of the web, and historically, we've wanted to keep it platform-independent, and let React be an abstraction under the hood, allowing 3rd party developers to work with a library of their choice. At the same time, full backward compatibility is one of our core values (we strive to minimize breaking changes), so we'll be obligated to maintain @wordpress/element forever, meaning that re-exporting will have to continue. Simultaneously, we wanted to be up to date with the latest React, and leverage React Compiler, so we're trying to find a way to make the best possible compromise.

Let me know if that makes sense!

We would generally expect that developers consuming React do so via our packages, and the compiler is optimized around that.

Worst case scenario, this might be something to consider after all, but I wanted to check if any alternatives are considered first.

I appreciate if you have any further thoughts or vision to share.

Thanks again!

@josephsavona
Copy link
Contributor

Thoughts on expanding TypeSchema to include something like BuiltInUseState | BuiltInUseRef etc, vs allowing library authors to add additional annotations to functions?

It's probably safer to start with the former and just allow type schemas to reference all of the builtin types. We can always expand to the former later, once we know what the set of attributes should be (I think we're about 90% confident of this right now but i'd rather defer canonicalizing this if possible)

@tyxla
Copy link
Author

tyxla commented Oct 11, 2024

In the meantime, we're discussing the feasibility of using react and react-dom directly, which will solve the abovementioned issue for us: WordPress/gutenberg#66063

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

Successfully merging this pull request may close these issues.

4 participants