-
Notifications
You must be signed in to change notification settings - Fork 48.7k
[compiler]: fix link compiler & 4 broken tests from path containing spaces #33409
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]: fix link compiler & 4 broken tests from path containing spaces #33409
Conversation
Comparing: 428ab82...6c525d1 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
…eu/react into fix-compiler-path-spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! A couple small things before we can land, see comments.
@@ -528,7 +528,7 @@ describe('ReactTypeScriptClass', function () { | |||
' in ProvideChildContextTypes (at **)', | |||
'StateBasedOnContext uses the legacy contextTypes API which will soon be removed. ' + | |||
'Use React.createContext() with static contextType instead. (https://react.dev/link/legacy-context)\n' + | |||
' in ProvideChildContextTypes.createElement (at **)', | |||
' in ProvideChildContextTypes.createElement [as render] (at **)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unrelated, let’s revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thank you for reviewing and for you spotting that. Fixed!
@@ -721,7 +721,7 @@ describe('ReactTypeScriptClass', function () { | |||
' in ProvideContext (at **)', | |||
'ReadContext uses the legacy contextTypes API which will soon be removed. ' + | |||
'Use React.createContext() with static contextType instead. (https://react.dev/link/legacy-context)\n' + | |||
' in ProvideContext.createElement (at **)', | |||
' in ProvideContext.createElement [as render] (at **)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too
thanks again! |
Summary
Problem #1: Running the
link-compiler.sh
bash script via"prebuild"
script fails if a developer has cloned thereact
repo into a folder that contains any spaces. 3 tests fail because of this.For example, my current folder is:
/Users/wes/Development/Open Source Contributions/react
The link compiler error returns:
./scripts/react-compiler/link-compiler.sh: line 15: cd: /Users/wes/Development/Open: No such file or directory
Problem #2: 1 test in
ReactChildren-test.js
fails due the existing stack trace regex which should be lightly revised.([^(\[\n]+)[^\n]*/g
is more robust for stack traces: it captures the function/class name (with dots) and does not break on spaces in file paths.([\S]+)[^\n]*/g
is simpler but breaks if there are spaces and doesn't handle dotted names well.Additionally, we trim the whitespace off the name to resolve extra spaces breaking this test as well:
All of the above tests pass if I hyphenate my local folder:
/Users/wes/Development/Open-Source-Contributions/react
I selfishly want to keep spaces in my folder names. 🫣
How did you test this change?
npx yarn prebuild
Before:

After:

npx yarn test
npx yarn test ./packages/react/src/__tests__/ReactChildren-test.js
npx yarn test -r=xplat --env=development --variant=true --ci --shard=3/5
Before:

After:
