Skip to content

[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

Merged
merged 19 commits into from
Jun 9, 2025

Conversation

wlemahieu
Copy link
Contributor

@wlemahieu wlemahieu commented Jun 3, 2025

Summary

Problem #1: Running the link-compiler.sh bash script via "prebuild" script fails if a developer has cloned the react repo into a folder that contains any spaces. 3 tests fail because of this.

fail-1 fail-2 fail-3

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:

-     in div (at **)
+     in div  (at **)
fail-4

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:
Screenshot at Jun 01 11-42-56

After:
Screenshot at Jun 01 11-43-42

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:
before

After:
after

Screenshot at Jun 02 18-03-39 Screenshot at Jun 03 12-53-47

@react-sizebot
Copy link

react-sizebot commented Jun 3, 2025

Comparing: 428ab82...6c525d1

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 530.07 kB 530.07 kB = 93.57 kB 93.57 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 651.16 kB 651.16 kB = 114.70 kB 114.70 kB
facebook-www/ReactDOM-prod.classic.js = 676.11 kB 676.11 kB = 118.97 kB 118.97 kB
facebook-www/ReactDOM-prod.modern.js = 666.39 kB 666.39 kB = 117.36 kB 117.36 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 6c525d1

@wlemahieu wlemahieu changed the title [compiler]: fix link-compiler.sh and 4 tests breaking on pwd /w spaces [compiler]: fix link compiler & 4 tests breaking /w paths containing spaces Jun 3, 2025
@wlemahieu wlemahieu changed the title [compiler]: fix link compiler & 4 tests breaking /w paths containing spaces [compiler]: fix link compiler & 4 broken tests from path containing spaces Jun 3, 2025
Copy link
Member

@josephsavona josephsavona left a 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 **)',
Copy link
Member

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?

Copy link
Contributor Author

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 **)',
Copy link
Member

Choose a reason for hiding this comment

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

Here too

@wlemahieu wlemahieu requested a review from josephsavona June 7, 2025 20:48
@josephsavona josephsavona merged commit b6c0aa8 into facebook:main Jun 9, 2025
253 checks passed
@josephsavona
Copy link
Member

thanks again!

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.

4 participants