Skip to content
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

[Fiber] InvokeGuardedCallback without metaprogramming #26569

Merged
merged 1 commit into from
Apr 20, 2023

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Apr 7, 2023

InvokeGuardedCallback is now implemented with the browser fork done at error-time rather than module-load-time. Originally it also tried to freeze the window/document references to avoid mismatches in prototype chains when testing React in different documents however we have since updated our tests to not do this and it was a test only feature so I removed it.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 7, 2023
@react-sizebot
Copy link

react-sizebot commented Apr 7, 2023

Comparing: fdad813...8e9965d

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.min.js = 163.95 kB 163.95 kB = 51.67 kB 51.67 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 167.70 kB 167.70 kB = 52.81 kB 52.81 kB
facebook-www/ReactDOM-prod.classic.js = 566.07 kB 566.07 kB = 100.25 kB 100.25 kB
facebook-www/ReactDOM-prod.modern.js = 549.80 kB 549.80 kB = 97.57 kB 97.57 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-dom/umd/react-dom-test-utils.development.js = 59.33 kB 58.31 kB = 16.56 kB 16.19 kB
oss-stable-semver/react-dom/umd/react-dom-test-utils.development.js = 59.33 kB 58.31 kB = 16.56 kB 16.19 kB
oss-stable/react-dom/umd/react-dom-test-utils.development.js = 59.33 kB 58.31 kB = 16.56 kB 16.19 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.development.js = 56.16 kB 55.15 kB = 16.28 kB 15.92 kB
oss-stable-semver/react-dom/cjs/react-dom-test-utils.development.js = 56.16 kB 55.15 kB = 16.28 kB 15.92 kB
oss-stable/react-dom/cjs/react-dom-test-utils.development.js = 56.16 kB 55.15 kB = 16.28 kB 15.92 kB

Generated by 🚫 dangerJS against 8e9965d

@gnoff gnoff force-pushed the icg-without-metaprogramming branch from 3b02d97 to 5713b4d Compare April 7, 2023 19:30
@gnoff gnoff requested a review from sebmarkbage April 7, 2023 19:30
@gnoff gnoff assigned acdlite and unassigned acdlite Apr 7, 2023
@gnoff gnoff requested a review from acdlite April 7, 2023 19:30
@gnoff gnoff force-pushed the icg-without-metaprogramming branch from 5713b4d to 9f3a8f7 Compare April 7, 2023 21:20
};
}
// We only get here if we are in an environment that either does not support the browser
// variant or we had trouble getting the brwoser to emit the error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// variant or we had trouble getting the brwoser to emit the error.
// variant or we had trouble getting the browser to emit the error.

@gnoff gnoff force-pushed the icg-without-metaprogramming branch 2 times, most recently from 86976ff to 87f74c6 Compare April 10, 2023 22:03
Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

We probably shouldn't rely on window/document changing anyway. I suspect this won't be the last time we do this. In fact, I'm kind of doing it now in my draft PR.

I feel like it's safe for us to assume that window/document won't change during the life time and it's up to us to rewrite our tests to not rely on it.

The implementation is largely unchanged except that the branching for when to use thebrowser specific override in dev is done in the function execution rather than by replacing the function implementation upon module evaluation.
@gnoff gnoff force-pushed the icg-without-metaprogramming branch from 87f74c6 to 8e9965d Compare April 20, 2023 22:03
@gnoff gnoff merged commit cc93a85 into facebook:main Apr 20, 2023
@gnoff gnoff deleted the icg-without-metaprogramming branch April 20, 2023 22:08
kassens pushed a commit that referenced this pull request Apr 21, 2023
InvokeGuardedCallback is now implemented with the browser fork done at
error-time rather than module-load-time. Originally it also tried to
freeze the window/document references to avoid mismatches in prototype
chains when testing React in different documents however we have since
updated our tests to not do this and it was a test only feature so I
removed it.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
InvokeGuardedCallback is now implemented with the browser fork done at
error-time rather than module-load-time. Originally it also tried to
freeze the window/document references to avoid mismatches in prototype
chains when testing React in different documents however we have since
updated our tests to not do this and it was a test only feature so I
removed it.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
InvokeGuardedCallback is now implemented with the browser fork done at
error-time rather than module-load-time. Originally it also tried to
freeze the window/document references to avoid mismatches in prototype
chains when testing React in different documents however we have since
updated our tests to not do this and it was a test only feature so I
removed it.

DiffTrain build for commit cc93a85.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants