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

Updating forking implementation to match against more general fork implementations #27205

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Aug 9, 2023

Search for more generic fork files if an exact match does not exist. If forks/MyFile.dom.js exists but forks/MyFile.dom-node.js does not then use it when trying to resolve forks for the "dom-node" renderer in flow, tests, and build

consolidate certain fork files that were identical and make semantic sense to be generalized
add dom-browser-esm bundle and use it for react-server-dom-esm/client.browser build

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 9, 2023
@gnoff gnoff changed the title Cascading forks Updating forking implementation to match against more general fork implementations Aug 9, 2023
@gnoff gnoff force-pushed the cascading-forks branch 4 times, most recently from 251aa28 to 43afdd6 Compare August 9, 2023 16:21
@react-sizebot
Copy link

react-sizebot commented Aug 9, 2023

Comparing: a20eea2...9b8e986

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 = 164.32 kB 164.32 kB = 51.76 kB 51.76 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.74 kB 171.74 kB = 53.97 kB 53.98 kB
facebook-www/ReactDOM-prod.classic.js = 567.12 kB 567.12 kB = 100.09 kB 100.09 kB
facebook-www/ReactDOM-prod.modern.js = 550.92 kB 550.92 kB = 97.25 kB 97.25 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js = 56.55 kB 54.01 kB = 13.63 kB 12.95 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js = 56.39 kB 53.85 kB = 13.57 kB 12.89 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js = 52.21 kB 49.66 kB = 12.74 kB 12.07 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js = 52.21 kB 49.66 kB = 12.74 kB 12.07 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js = 52.04 kB 49.50 kB = 12.68 kB 12.02 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js = 52.04 kB 49.50 kB = 12.68 kB 12.02 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js = 44.30 kB 41.76 kB = 10.85 kB 10.16 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js = 39.95 kB 37.41 kB = 9.95 kB 9.26 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js = 39.95 kB 37.41 kB = 9.95 kB 9.26 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.min.js = 11.42 kB 10.62 kB = 4.35 kB 4.05 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.min.js = 10.39 kB 9.59 kB = 4.03 kB 3.73 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.min.js = 10.39 kB 9.59 kB = 4.03 kB 3.73 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-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js = 56.55 kB 54.01 kB = 13.63 kB 12.95 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js = 56.39 kB 53.85 kB = 13.57 kB 12.89 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js = 52.21 kB 49.66 kB = 12.74 kB 12.07 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.development.js = 52.21 kB 49.66 kB = 12.74 kB 12.07 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js = 52.04 kB 49.50 kB = 12.68 kB 12.02 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.development.js = 52.04 kB 49.50 kB = 12.68 kB 12.02 kB
oss-experimental/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js = 44.30 kB 41.76 kB = 10.85 kB 10.16 kB
oss-stable-semver/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js = 39.95 kB 37.41 kB = 9.95 kB 9.26 kB
oss-stable/react-server-dom-esm/esm/react-server-dom-esm-client.browser.production.min.js = 39.95 kB 37.41 kB = 9.95 kB 9.26 kB
oss-experimental/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.min.js = 11.42 kB 10.62 kB = 4.35 kB 4.05 kB
oss-stable-semver/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.min.js = 10.39 kB 9.59 kB = 4.03 kB 3.73 kB
oss-stable/react-server-dom-esm/cjs/react-server-dom-esm-client.browser.production.min.js = 10.39 kB 9.59 kB = 4.03 kB 3.73 kB

Generated by 🚫 dangerJS against 9b8e986

@gnoff gnoff force-pushed the cascading-forks branch from 43afdd6 to 73cfdc0 Compare August 9, 2023 19:33
@gnoff gnoff requested a review from sebmarkbage August 9, 2023 19:34
@gnoff gnoff force-pushed the cascading-forks branch from 73cfdc0 to 5a9fd27 Compare August 9, 2023 19:35
…t found. for instance `File.dom` can match when using renderer `"dom-node"` if `File.dom-node` does not exist.

This change also splits the browser and node builds for react-server-dom-esm into two separate renderers. There is not a `"dom-browser-esm"` renderer.

This change should alter no semantics. It does however set us up to support more complex and varied renderer configs in future updates and generally lowers the effort to spin up additional configs.
@gnoff gnoff force-pushed the cascading-forks branch from 5a9fd27 to 9b8e986 Compare August 9, 2023 19:37
export * from 'react-client/src/ReactFlightClientConfigBrowser';
export * from 'react-server-dom-esm/src/ReactFlightClientConfigESMBundler';
export * from 'react-dom-bindings/src/shared/ReactFlightClientConfigDOM';
export const usedWithSSR = false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for the browser build usedWithSSR should have always been false, but now that we can separate these renderers we can slightly optimize the build with this config difference

@@ -7,7 +7,6 @@
* @flow
*/

// This should really have a Node and a Browser fork but to avoid too many configs we limit this to build the same for both
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dom-browser-esm now exists

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

all dom based ReactFiberConfigs have been consolidated in ReactFiberConfig.dom.js

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same impl as ReactFizzConfig.dom

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not webpack specific so renaming .dom-edge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same impl as ReactFizzConfig.dom-node

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Git is showing this as a rename but really we deleted the dom-node-webpack to pickup the dom-node version and we added dom-browser-esm for that new renderer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

stream configs don't depend on webpack vs esm. using dom-node to generalize

Comment on lines +495 to +497
console.error(
chalk.red(`There was an error preparing plugins for entry "${entry}"`)
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is helpful when a build fails in CI. currently you cannot tell which one failed, just the ones that succeeded before it

Comment on lines +393 to +397
return new Error(
`Expected not to use ReactFlightServerConfig with "${entry}" entry point ` +
'in ./scripts/shared/inlinedHostConfigs.js. Update the renderer config to ' +
'activate flight suppport and add a matching fork implementation for ReactFlightServerConfig.'
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

previously this would return a fork to a non-existent file such as ReactFlightServerConfig.dom-fb.js but since that file was never referenced there was no error. To mimic this we return an error here when we don't fine a file as long as the bundle declares that it does not implement Flight.

@gnoff gnoff merged commit 5623f2a into facebook:main Aug 17, 2023
@gnoff gnoff deleted the cascading-forks branch August 17, 2023 22:17
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…plementations (facebook#27205)

Search for more generic fork files if an exact match does not exist. If
`forks/MyFile.dom.js` exists but `forks/MyFile.dom-node.js` does not
then use it when trying to resolve forks for the `"dom-node"` renderer
in flow, tests, and build

consolidate certain fork files that were identical and make semantic
sense to be generalized
add `dom-browser-esm` bundle and use it for
`react-server-dom-esm/client.browser` build
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…plementations (#27205)

Search for more generic fork files if an exact match does not exist. If
`forks/MyFile.dom.js` exists but `forks/MyFile.dom-node.js` does not
then use it when trying to resolve forks for the `"dom-node"` renderer
in flow, tests, and build

consolidate certain fork files that were identical and make semantic
sense to be generalized
add `dom-browser-esm` bundle and use it for
`react-server-dom-esm/client.browser` build

DiffTrain build for commit 5623f2a.
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.

4 participants