-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Babel-ignore node_modules
on Windows consistently with others
#42681
Closed
+1
−1
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
facebook-github-bot
added
the
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
label
Jan 26, 2024
This pull request was exported from Phabricator. Differential Revision: D53124578 |
robhogan
added a commit
to robhogan/react-native
that referenced
this pull request
Jan 26, 2024
…hers (facebook#42681) Summary: CI failures in Windows JS tests recently (facebook#41463) were caused by the triggering of Babel registration during tests, due to an import of `packages/dev-middleware` (index), breaking subsequent transformation of other tests. ## Root cause Example of a problematic import: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/__tests__/ServerUtils.js#L15 ..which triggers a Babel registration: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/index.js#L16-L18 That registration behaves differently on Windows due to the `ignore: [/\/node_modules\/\]`, which doesn't match against Windows path separators - Babel matches against system separators. In particular, this changed whether `node_modules/flow-parser` was transformed when loading the RN Babel transformer. Transforming this file causes a `console.warn` from Babel due to its size: > [BABEL] Note: The code generator has deoptimised the styling of /Users/robhogan/workspace/react-native/node_modules/flow-parser/flow_parser.js as it exceeds the max of 500KB. This throws due to our setup: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/react-native/jest/local-setup.js#L27 This all manifests as the first test following a Babel registration (within the same Jest worker) that requires the RN Babel transformer throwing during script transformation. ## This change This is the minimally disruptive change that makes Babel registration behaviour consistent between Windows and other platforms. The more durable solution here would be *not* to rely on any Babel registration for Jest, which has its own `ScriptTransformer` mechanism for running code from source. Given the fragile way our internal+OSS Babel set up hangs together that's a higher-risk change, so I'll follow up separately. Changelog: [Internal] Differential Revision: D53124578
robhogan
added a commit
to robhogan/react-native
that referenced
this pull request
Jan 27, 2024
…hers (facebook#42681) Summary: CI failures in Windows JS tests recently (facebook#41463) were caused by the triggering of Babel registration during tests, due to an import of `packages/dev-middleware` (index), breaking subsequent transformation of other tests. ## Root cause Example of a problematic import: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/__tests__/ServerUtils.js#L15 ..which triggers a Babel registration: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/index.js#L16-L18 That registration behaves differently on Windows due to the `ignore: [/\/node_modules\/\]`, which doesn't match against Windows path separators - Babel matches against system separators. In particular, this changed whether `node_modules/flow-parser` was transformed when loading the RN Babel transformer. Transforming this file causes a `console.warn` from Babel due to its size: > [BABEL] Note: The code generator has deoptimised the styling of /Users/robhogan/workspace/react-native/node_modules/flow-parser/flow_parser.js as it exceeds the max of 500KB. This throws due to our setup: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/react-native/jest/local-setup.js#L27 This all manifests as the first test following a Babel registration (within the same Jest worker) that requires the RN Babel transformer throwing during script transformation. ## This change This is the minimally disruptive change that makes Babel registration behaviour consistent between Windows and other platforms. The more durable solution here would be *not* to rely on any Babel registration for Jest, which has its own `ScriptTransformer` mechanism for running code from source. Given the fragile way our internal+OSS Babel set up hangs together that's a higher-risk change, so I'll follow up separately. Changelog: [Internal] Reviewed By: huntie Differential Revision: D53124578
…book#42681) Summary: CI failures in Windows JS tests recently (facebook#41463) were caused by the triggering of Babel registration during tests, due to an import of `packages/dev-middleware` (index), breaking subsequent transformation of other tests. ## Root cause Example of a problematic import: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/__tests__/ServerUtils.js#L15 ..which triggers a Babel registration: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/index.js#L16-L18 That registration behaves differently on Windows due to the `ignore: [/\/node_modules\/\]`, which doesn't match against Windows path separators - Babel matches against system separators. In particular, this changed whether `node_modules/flow-parser` was transformed when loading the RN Babel transformer. Transforming this file causes a `console.warn` from Babel due to its size: > [BABEL] Note: The code generator has deoptimised the styling of /Users/robhogan/workspace/react-native/node_modules/flow-parser/flow_parser.js as it exceeds the max of 500KB. This throws due to our setup: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/react-native/jest/local-setup.js#L27 This all manifests as the first test following a Babel registration (within the same Jest worker) that requires the RN Babel transformer throwing during script transformation. ## This change This is the minimally disruptive change that makes Babel registration behaviour consistent between Windows and other platforms. The more durable solution here would be *not* to rely on any Babel registration for Jest, which has its own `ScriptTransformer` mechanism for running code from source. Given the fragile way our internal+OSS Babel set up hangs together that's a higher-risk change, so I'll follow up separately. Changelog: [Internal] Reviewed By: huntie Differential Revision: D53124578
robhogan
force-pushed
the
export-D53124578
branch
from
January 27, 2024 14:16
1c6f181
to
dc199f7
Compare
robhogan
added a commit
to robhogan/react-native
that referenced
this pull request
Jan 27, 2024
…hers (facebook#42681) Summary: CI failures in Windows JS tests recently (facebook#41463) were caused by the triggering of Babel registration during tests, due to an import of `packages/dev-middleware` (index), breaking subsequent transformation of other tests. ## Root cause Example of a problematic import: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/__tests__/ServerUtils.js#L15 ..which triggers a Babel registration: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/dev-middleware/src/index.js#L16-L18 That registration behaves differently on Windows due to the `ignore: [/\/node_modules\/\]`, which doesn't match against Windows path separators - Babel matches against system separators. In particular, this changed whether `node_modules/flow-parser` was transformed when loading the RN Babel transformer. Transforming this file causes a `console.warn` from Babel due to its size: > [BABEL] Note: The code generator has deoptimised the styling of /Users/robhogan/workspace/react-native/node_modules/flow-parser/flow_parser.js as it exceeds the max of 500KB. This throws due to our setup: https://github.com/facebook/react-native/blob/a5d8ea4579c630af1e4e0fe1d99ad9dc0915df86/packages/react-native/jest/local-setup.js#L27 This all manifests as the first test following a Babel registration (within the same Jest worker) that requires the RN Babel transformer throwing during script transformation. ## This change This is the minimally disruptive change that makes Babel registration behaviour consistent between Windows and other platforms. The more durable solution here would be *not* to rely on any Babel registration for Jest, which has its own `ScriptTransformer` mechanism for running code from source. Given the fragile way our internal+OSS Babel set up hangs together that's a higher-risk change, so I'll follow up separately. Changelog: [Internal] Reviewed By: huntie Differential Revision: D53124578
This pull request was exported from Phabricator. Differential Revision: D53124578 |
This pull request has been merged in 41b256a. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
CLA Signed
This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
fb-exported
Merged
This PR has been merged.
p: Facebook
Partner: Facebook
Partner
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.
Summary:
CI failures in Windows JS tests recently (#41463) were caused by the triggering of Babel registration during tests, due to an import of
packages/dev-middleware
(index), breaking subsequent transformation of other tests.Root cause
Example of a problematic import:
react-native/packages/dev-middleware/src/__tests__/ServerUtils.js
Line 15 in a5d8ea4
..which triggers a Babel registration:
react-native/packages/dev-middleware/src/index.js
Lines 16 to 18 in a5d8ea4
That registration behaves differently on Windows due to the
ignore: [/\/node_modules\/\]
, which doesn't match against Windows path separators - Babel matches against system separators.In particular, this changed whether
node_modules/flow-parser
was transformed when loading the RN Babel transformer. Transforming this file causes aconsole.warn
from Babel due to its size:This throws due to our setup:
react-native/packages/react-native/jest/local-setup.js
Line 27 in a5d8ea4
This all manifests as the first test following a Babel registration (within the same Jest worker) that requires the RN Babel transformer throwing during script transformation.
This change
This is the minimally disruptive change that makes Babel registration behaviour consistent between Windows and other platforms. The more durable solution here would be not to rely on any Babel registration for Jest, which has its own
ScriptTransformer
mechanism for running code from source. Given the fragile way our internal+OSS Babel set up hangs together that's a higher-risk change, so I'll follow up separately.Changelog: [Internal]
Differential Revision: D53124578