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

feat(babel-plugin-react-compiler): support satisfies operator #32742

Conversation

rodrigofariow
Copy link
Contributor

@rodrigofariow rodrigofariow commented Mar 25, 2025

Solve #29818

@rodrigofariow
Copy link
Contributor Author

rodrigofariow commented Mar 25, 2025

Hi @josephsavona , this should be a relatively simple change.
I added tests based on the AS operator. It's very similar if not the same.

The problem right now is that because of a simple "satisfies", an entire component is no longer memoizable by the compiler.

I tested this change as a patch on my codebase and it worked fine on the output. Instead of skipping a component, it now compiles it fine.

What can I do to have this merged?

Cheers

@josephsavona
Copy link
Contributor

Thanks for the contribution. The changes and test look reasonable, can you fix the test and lint issues?

@rodrigofariow
Copy link
Contributor Author

rodrigofariow commented Mar 25, 2025

Fixed formatting ✅
Thank you for the fast feedback btw @josephsavona 👍

mofeiZ added a commit that referenced this pull request Mar 26, 2025
Currently, `babel-plugin-react-compiler` is bundled with (almost) all external dependencies. This is because babel traversal and ast logic is not forward-compatible. Since `babel-plugin-react-compiler` needs to be compatible with babel pipelines across a wide semvar range, we (1) set this package's babel dependency to an early version and (2) inline babel libraries into our bundle.

A few other packages in `react/compiler` depend on the compiler. This PR moves `snap`, our test fixture compiler and evaluator, to use the bundled version of `babel-plugin-react-compiler`. This decouples the babel version used by `snap` with the version used by `babel-plugin-react-compiler`, which means that `snap` now can test features from newer babel versions (see #32742).
mofeiZ added a commit that referenced this pull request Mar 26, 2025
Currently, `babel-plugin-react-compiler` is bundled with (almost) all external dependencies. This is because babel traversal and ast logic is not forward-compatible. Since `babel-plugin-react-compiler` needs to be compatible with babel pipelines across a wide semvar range, we (1) set this package's babel dependency to an early version and (2) inline babel libraries into our bundle.

A few other packages in `react/compiler` depend on the compiler. This PR moves `snap`, our test fixture compiler and evaluator, to use the bundled version of `babel-plugin-react-compiler`. This decouples the babel version used by `snap` with the version used by `babel-plugin-react-compiler`, which means that `snap` now can test features from newer babel versions (see #32742).
mofeiZ added a commit that referenced this pull request Mar 26, 2025
Currently, `babel-plugin-react-compiler` is bundled with (almost) all external dependencies. This is because babel traversal and ast logic is not forward-compatible. Since `babel-plugin-react-compiler` needs to be compatible with babel pipelines across a wide semvar range, we (1) set this package's babel dependency to an early version and (2) inline babel libraries into our bundle.

A few other packages in `react/compiler` depend on the compiler. This PR moves `snap`, our test fixture compiler and evaluator, to use the bundled version of `babel-plugin-react-compiler`. This decouples the babel version used by `snap` with the version used by `babel-plugin-react-compiler`, which means that `snap` now can test features from newer babel versions (see #32742).
mofeiZ added a commit that referenced this pull request Mar 26, 2025
Currently, `babel-plugin-react-compiler` is bundled with (almost) all
external dependencies. This is because babel traversal and ast logic is
not forward-compatible. Since `babel-plugin-react-compiler` needs to be
compatible with babel pipelines across a wide semvar range, we (1) set
this package's babel dependency to an early version and (2) inline babel
libraries into our bundle.

A few other packages in `react/compiler` depend on the compiler. This PR
moves `snap`, our test fixture compiler and evaluator, to use the
bundled version of `babel-plugin-react-compiler`. This decouples the
babel version used by `snap` with the version used by
`babel-plugin-react-compiler`, which means that `snap` now can test
features from newer babel versions (see
#32742).

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32758).
* #32759
* __->__ #32758
github-actions bot pushed a commit that referenced this pull request Mar 26, 2025
Currently, `babel-plugin-react-compiler` is bundled with (almost) all
external dependencies. This is because babel traversal and ast logic is
not forward-compatible. Since `babel-plugin-react-compiler` needs to be
compatible with babel pipelines across a wide semvar range, we (1) set
this package's babel dependency to an early version and (2) inline babel
libraries into our bundle.

A few other packages in `react/compiler` depend on the compiler. This PR
moves `snap`, our test fixture compiler and evaluator, to use the
bundled version of `babel-plugin-react-compiler`. This decouples the
babel version used by `snap` with the version used by
`babel-plugin-react-compiler`, which means that `snap` now can test
features from newer babel versions (see
#32742).

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32758).
* #32759
* __->__ #32758

DiffTrain build for [33999c4](33999c4)
@mofeiZ
Copy link
Contributor

mofeiZ commented Mar 26, 2025

Thanks for the contribution! @rodrigofariow you should be able to run tests after rebasing on #32758

# in compiler/packages/babel-plugin-react-compiler
yarn snap:build && yarn snap --watch

Note that you'll probably want to make changes in CodegenReactiveFunctions.ts as this currently replaces satisfies TSExpressions with as casts

@rodrigofariow rodrigofariow force-pushed the babel-plugin-react-compiler-support-satisfies-operator branch from 6595f69 to 472e40c Compare March 27, 2025 15:37
codegenPlaceToExpression(cx, instrValue.value),
instrValue.typeAnnotation,
);
if (t.isTSSatisfiesExpression(instrValue.typeAnnotation)) {
Copy link
Contributor Author

@rodrigofariow rodrigofariow Mar 27, 2025

Choose a reason for hiding this comment

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

Hi @mofeiZ
Thanks for the info.
Is this the change you were suggesting? Makes sense in theory but I'm not 100% sure 😆

let expr = exprPath as NodePath<t.TSAsExpression>;
let expr = exprPath as NodePath<
t.TSAsExpression | t.TSSatisfiesExpression
>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm the problem here is that we're not storing the TSAsExpression | TSSatisfiesExpression node themselves in our IR. Instead, we store the typeAnnotation (e.g. number part of satisfies number).

You'll want to make a change in HIR.ts -- you can add a typeAnnotationKind: 'as' | 'satisfies' property to the TypeCastExpression instruction.

Also I noticed that your test fixtures don't seem to be updated in the commits. Could you try yarn snap --watch and yarn snap --update locally so you get signal before submitting? Let me know if you have trouble with this (I know we hardcode linux paths so it may not be compatible)

typeAnnotation: t.FlowType;
typeAnnotationKind: 'cast';
}
| {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to make it so that it wouldn't be possible to have a 'satisfies' annotation kind when the annotation was t.FlowType.
Hence why I opted for this slightly more complex union FYI @mofeiZ

Copy link
Contributor

@mofeiZ mofeiZ left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for the contribution!

@mofeiZ mofeiZ merged commit ef4bc8b into facebook:main Mar 28, 2025
21 checks passed
@rodrigofariow
Copy link
Contributor Author

Thank you for your guidance @mofeiZ @josephsavona! 🙏

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.

5 participants