[ci] Reduce non-deterministic builds for eslint-plugin-react-hooks #33026
Conversation
This file contains hidden or 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
See rollup/plugins#1425 Currently, `@babel/helper-string-parser/lib/index.js` is either emitted as a wrapped esmodule or inline depending on the ordering of async functions in `rollup/commonjs`. Specifically, `@babel/types/lib/definitions/core.js` is cyclic (i.e. transitively depends upon itself), but sometimes `@babel/helper-string-parser/lib/index.js` is emitted before this is realized. A relatively straightforward patch is to wrap all modules (see rollup/plugins#1425 (comment))
There was a problem hiding this comment.
Pull Request Overview
This PR wraps all CommonJS modules with a stricter configuration to mitigate non-deterministic builds for eslint-plugin-react-hooks.
- Updated the commonjs plugin invocation to include {strictRequires: true} when bundle.tsconfig exists
- Added an inline comment with a reference to the relevant GitHub issue for context
Comments suppressed due to low confidence (1)
scripts/rollup/build.js:397
- Ensure that the 'strictRequires: true' configuration is fully supported by the current version of @rollup/commonjs and has been tested under all expected module ordering scenarios, given its impact on bundling behavior.
bundle.tsconfig != null ? commonjs({strictRequires: true}) : false,
github-actions bot
pushed a commit
that referenced
this pull request
Apr 25, 2025
…33026) See rollup/plugins#1425 Currently, `@babel/helper-string-parser/lib/index.js` is either emitted as a wrapped esmodule or inline depending on the ordering of async functions in `rollup/commonjs`. Specifically, `@babel/types/lib/definitions/core.js` is cyclic (i.e. transitively depends upon itself), but sometimes `@babel/helper-string-parser/lib/index.js` is emitted before this is realized. A relatively straightforward patch is to wrap all modules (see rollup/plugins#1425 (comment)). This only regresses `eslint-plugin-react-hooks` bundle size by ~1.8% and is safer (see https://github.com/rollup/plugins/blob/master/packages/commonjs/README.md#strictrequires) > The default value of true will wrap all CommonJS files in functions which are executed when they are required for the first time, preserving NodeJS semantics. This is the safest setting and should be used if the generated code does not work correctly with "auto". Note that strictRequires: true can have a small impact on the size and performance of generated code, but less so if the code is minified. (note that we're on an earlier version of `@rollup/commonjs` which does not default to `strictRequires: true`) DiffTrain build for [0c28a09](0c28a09)
github-actions bot
pushed a commit
to code/lib-react
that referenced
this pull request
Apr 25, 2025
…acebook#33026) See rollup/plugins#1425 Currently, `@babel/helper-string-parser/lib/index.js` is either emitted as a wrapped esmodule or inline depending on the ordering of async functions in `rollup/commonjs`. Specifically, `@babel/types/lib/definitions/core.js` is cyclic (i.e. transitively depends upon itself), but sometimes `@babel/helper-string-parser/lib/index.js` is emitted before this is realized. A relatively straightforward patch is to wrap all modules (see rollup/plugins#1425 (comment)). This only regresses `eslint-plugin-react-hooks` bundle size by ~1.8% and is safer (see https://github.com/rollup/plugins/blob/master/packages/commonjs/README.md#strictrequires) > The default value of true will wrap all CommonJS files in functions which are executed when they are required for the first time, preserving NodeJS semantics. This is the safest setting and should be used if the generated code does not work correctly with "auto". Note that strictRequires: true can have a small impact on the size and performance of generated code, but less so if the code is minified. (note that we're on an earlier version of `@rollup/commonjs` which does not default to `strictRequires: true`) DiffTrain build for [0c28a09](facebook@0c28a09)
This was referenced Apr 28, 2025
This was referenced May 5, 2025
This was referenced May 12, 2025
This was referenced May 19, 2025
17 tasks
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.
See rollup/plugins#1425
Currently,
@babel/helper-string-parser/lib/index.jsis either emitted as a wrapped esmodule or inline depending on the ordering of async functions inrollup/commonjs. Specifically,@babel/types/lib/definitions/core.jsis cyclic (i.e. transitively depends upon itself), but sometimes@babel/helper-string-parser/lib/index.jsis emitted before this is realized.A relatively straightforward patch is to wrap all modules (see rollup/plugins#1425 (comment)). This only regresses
eslint-plugin-react-hooksbundle size by ~1.8% and is safer (see https://github.com/rollup/plugins/blob/master/packages/commonjs/README.md#strictrequires)(note that we're on an earlier version of
@rollup/commonjswhich does not default tostrictRequires: true)