feat(eslint-plugin-react-hooks): merge rule from eslint-plugin-react-compiler into react-hooks plugin
#32416
+1,036
−604
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
185347d to
bf841c6
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
bf841c6 to
0b1f3a0
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
0b1f3a0 to
1e2d02b
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
1e2d02b to
dfd79bd
Compare
michaelfaith
added a commit
to michaelfaith/react
that referenced
this pull request
Feb 22, 2025
In preparation for the merging of the compiler plugin into this one (facebook#32416), this change proactively updates the plugin's `engines` declaration to require Node versions greater than or equal to 18 BREAKING CHANGE
michaelfaith
added a commit
to michaelfaith/react
that referenced
this pull request
Feb 22, 2025
In preparation for the merging of the compiler plugin into this one (facebook#32416), this change proactively updates the plugin's `engines` declaration to require Node versions greater than or equal to 18 BREAKING CHANGE
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
5261b0f to
f920d88
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
f920d88 to
6cd87fc
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
79b148c to
639cd8f
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
639cd8f to
011250b
Compare
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
011250b to
ab2a7e1
Compare
|
Looking into why some of the tests are failing in CI but not locally
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This change adds the new dependencies needed for merging the react compiler plugin in.
Update unit tests to use v8 rule tester for compiler tests
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
fc91617 to
7773b5c
Compare
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It it is a little bit slower now because we also generate d.ts files, but it's still much faster than rollup which we used prior to esbuild. ``` # build all compiler packages $ time yarn build ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It it is a little bit slower now because we also generate d.ts files, but it's still much faster than rollup which we used prior to esbuild. ``` # BEFORE: build all compiler packages $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # AFTER: build all compiler packages $ time yarn build ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It is slower when generating d.ts files, but it's still much faster than rollup which we used prior to esbuild. For now, I have turned off `dts` by default, and it is only passed when publishing on npm. If you want to also generate d.ts files you can run `yarn build --dts`. ``` # BEFORE: build all compiler packages $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # --- # AFTER: build all compiler packages $ time yarn build ✨ Done in 12.39s. yarn build 12.58s user 1.68s system 106% cpu 13.350 total # --- # AFTER: build all compiler packages and type declarations $ time yarn build --dts ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. This is primarily because React's build process uses rollup + rollup-plugin-typescript, which runs tsc. So the merged plugin needs to typecheck properly in order to build. An alternative might be to migrate to something like babel with rollup instead to simply strip types rather than typecheck before building. The minor downside of that approach is that we would need to manually maintain a d.ts file for eslint-plugin-react-hooks. For now I would like to see if this PR helps us make progress rather than go for the slightly worse alternative. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It is slower when generating d.ts files, but it's still much faster than rollup which we used prior to esbuild. For now, I have turned off `dts` by default, and it is only passed when publishing on npm. If you want to also generate d.ts files you can run `yarn build --dts`. ``` # BEFORE: build all compiler packages (esbuild) $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # --- # AFTER: build all compiler packages (tsup) $ time yarn build ✨ Done in 12.39s. yarn build 12.58s user 1.68s system 106% cpu 13.350 total # --- # AFTER: build all compiler packages and type declarations (tsup) $ time yarn build --dts ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. This is primarily because React's build process uses rollup + rollup-plugin-typescript, which runs tsc. So the merged plugin needs to typecheck properly in order to build. An alternative might be to migrate to something like babel with rollup instead to simply strip types rather than typecheck before building. The minor downside of that approach is that we would need to manually maintain a d.ts file for eslint-plugin-react-hooks. For now I would like to see if this PR helps us make progress rather than go for the slightly worse alternative. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It is slower when generating d.ts files, but it's still much faster than rollup which we used prior to esbuild. For now, I have turned off `dts` by default, and it is only passed when publishing on npm. If you want to also generate d.ts files you can run `yarn build --dts`. ``` # BEFORE: build all compiler packages (esbuild) $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # --- # AFTER: build all compiler packages (tsup) $ time yarn build ✨ Done in 12.39s. yarn build 12.58s user 1.68s system 106% cpu 13.350 total # --- # AFTER: build all compiler packages and type declarations (tsup) $ time yarn build --dts ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. This is primarily because React's build process uses rollup + rollup-plugin-typescript, which runs tsc. So the merged plugin needs to typecheck properly in order to build. An alternative might be to migrate to something like babel with rollup instead to simply strip types rather than typecheck before building. The minor downside of that approach is that we would need to manually maintain a d.ts file for eslint-plugin-react-hooks. For now I would like to see if this PR helps us make progress rather than go for the slightly worse alternative. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It is slower when generating d.ts files, but it's still much faster than rollup which we used prior to esbuild. For now, I have turned off `dts` by default, and it is only passed when publishing on npm. If you want to also generate d.ts files you can run `yarn build --dts`. ``` # BEFORE: build all compiler packages (esbuild) $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # --- # AFTER: build all compiler packages (tsup) $ time yarn build ✨ Done in 12.39s. yarn build 12.58s user 1.68s system 106% cpu 13.350 total # --- # AFTER: build all compiler packages and type declarations (tsup) $ time yarn build --dts ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. This is primarily because React's build process uses rollup + rollup-plugin-typescript, which runs tsc. So the merged plugin needs to typecheck properly in order to build. An alternative might be to migrate to something like babel with rollup instead to simply strip types rather than typecheck before building. The minor downside of that approach is that we would need to manually maintain a d.ts file for eslint-plugin-react-hooks. For now I would like to see if this PR helps us make progress rather than go for the slightly worse alternative. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It is slower when generating d.ts files, but it's still much faster than rollup which we used prior to esbuild. For now, I have turned off `dts` by default, and it is only passed when publishing on npm. If you want to also generate d.ts files you can run `yarn build --dts`. ``` # BEFORE: build all compiler packages (esbuild) $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # --- # AFTER: build all compiler packages (tsup) $ time yarn build ✨ Done in 12.39s. yarn build 12.58s user 1.68s system 106% cpu 13.350 total # --- # AFTER: build all compiler packages and type declarations (tsup) $ time yarn build --dts ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive.
poteto
added a commit
that referenced
this pull request
Mar 7, 2025
Currently in the `compiler` workspace, we invoke esbuild directly to build most packages (with the exception of `snap`). This has been mostly fine, but does not allow us to do things like generate type declaration files. I would like #32416 to be able to consume the merged eslint-plugin-react-compiler from source rather than via npm, and one of the things that has come up from my exploration in that stack using the compiler from source is that babel-plugin-react-compiler is missing type declarations. This is primarily because React's build process uses rollup + rollup-plugin-typescript, which runs tsc. So the merged plugin needs to typecheck properly in order to build. An alternative might be to migrate to something like babel with rollup instead to simply strip types rather than typecheck before building. The minor downside of that approach is that we would need to manually maintain a d.ts file for eslint-plugin-react-hooks. For now I would like to see if this PR helps us make progress rather than go for the slightly worse alternative. [`tsup`](https://github.com/egoist/tsup) is esbuild based so build performance is comparable. It is slower when generating d.ts files, but it's still much faster than rollup which we used prior to esbuild. For now, I have turned off `dts` by default, and it is only passed when publishing on npm. If you want to also generate d.ts files you can run `yarn build --dts`. ``` # BEFORE: build all compiler packages (esbuild) $ time yarn build ✨ Done in 15.61s. yarn build 13.82s user 1.54s system 96% cpu 15.842 total # --- # AFTER: build all compiler packages (tsup) $ time yarn build ✨ Done in 12.39s. yarn build 12.58s user 1.68s system 106% cpu 13.350 total # --- # AFTER: build all compiler packages and type declarations (tsup) $ time yarn build --dts ✨ Done in 30.69s. yarn build 43.57s user 3.20s system 150% cpu 31.061 total ``` I still need to test if this unblocks #32416 but this stack can be landed independently though as we could probably just release type declarations on npm. No one should be using the compiler directly, but if they really wanted to, lack of type declarations would not stop them (cf React secret internals). Note that I still kept esbuild as we still use it directly for forgive. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32550). * #32551 * __->__ #32550
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
11bab0a to
a5364eb
Compare
…e specific dep This change reverts the attempt to use the compiler source directly as part of unit testing, and instead installs an already published version of the package. The implication here is that it's not possible to consume unreleased / unpublished changes to the compiler in the eslint plugin. There's a pretty big disconnect between packages in the `compiler` folder and everything. So bridging that gap will take some additional work. build(eslint-plugin-react-hooks): remove jest modulenamemapper and use specific dep This change reverts the attempt to use the compiler source directly as part of unit testing, and instead installs an already published version of the package. The implication here is that it's not possible to consume unreleased / unpublished changes to the compiler in the eslint plugin. There's a pretty big disconnect between packages in the `compiler` folder and everything. So bridging that gap will take some additional work.
…ion is on the left side of an AssignmentExpression before calling lowerAssignment
michaelfaith
force-pushed
the
feat/merge-compiler-eslint-plugin
branch
from
a5364eb to
b00503e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change merges the
react-compilerrule fromeslint-plugin-react-compilerinto theeslint-plugin-react-hooksplugin. In order to do the move in a way that keeps commit history with the moved files, but also no remove them from their origin until a future cleanup change can be done, I did thegit mvfirst, and then recreated the files that were moved in their original places, as a separate commit. Unfortunately GH shows the moved files as new instead of the ones that are truly new. But in the IDE andgit blame, commit history is intact with the moved files.Since this change adds new dependencies, and one of those dependencies has a higher
enginesdeclaration fornodethan what the plugin currently has, this is technically a breaking change and will have to go out as part of a major release.Related Changes
enginesdeclaration #32458