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.

Sign up for GitHub

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

Jump to bottom

Fixed ReactSharedInternals export in UMD bundle #22117

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Fixed ReactSharedInternals export in UMD bundle #22117

merged 1 commit into from
Aug 18, 2021

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 17, 2021 •
edited
Loading

Resolves #22113

Prior to this PR, the react UMD bundle contained two ReactSharedInternals objects. The first ReactSharedInternals object came from here:

react/packages/react/src/ReactSharedInternals.js

Lines 15 to 26 in bd25570

const ReactSharedInternals = {
ReactCurrentDispatcher,
ReactCurrentBatchConfig,
ReactCurrentOwner,
// Used by renderers to avoid bundling object-assign twice in UMD bundles:
assign,
};
if (__DEV__) {
ReactSharedInternals.ReactDebugCurrentFrame = ReactDebugCurrentFrame;
ReactSharedInternals.ReactCurrentActQueue = ReactCurrentActQueue;
}

The second ReactSharedInternals object (ReactSharedInternals$1 in the bundle) came from here:

react/packages/react/src/forks/ReactSharedInternals.umd.js

Lines 15 to 28 in bd25570

const ReactSharedInternals = {
ReactCurrentDispatcher,
ReactCurrentOwner,
ReactCurrentBatchConfig,
// Used by renderers to avoid bundling object-assign twice in UMD bundles:
assign,
// Re-export the schedule API(s) for UMD bundles.
// This avoids introducing a dependency on a new UMD global in a minor update,
// Since that would be a breaking change (e.g. for all existing CodeSandboxes).
// This re-export is only required for UMD bundles;
// CJS bundles use the shared NPM package.
Scheduler,
};

The second one is the one that ended up being shared via __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED. It had a Scheduler key but not a ReactCurrentActQueue key. The fix was to make sure both keys got added to the UMD override.

Testing

Re-built the bundle:

node scripts/rollup/build.js --type=UMD_DEV -- react/index,react-dom/index

Then inspected the output at:

build/node_modules/react/umd/react.development.js
build/node_modules/react-dom/umd/react-dom.development.js

Also dropped both into a small repro app:

<!DOCTYPE html>
<html>
  <head>
    <script src="https://unpkg.com/react@alpha/umd/react.development.js"></script>
    <script src="https://unpkg.com/react-dom@alpha/umd/react-dom.development.js"></script>
  </head>
  <body>
    <script type="text/javascript">
      const container = document.createElement('div');

      document.body.appendChild(container);

      const root = ReactDOM.createRoot(container);
      root.render(React.createElement('div', null, 'Testing...'));
    </script>
  </body>
</html>

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 17, 2021
@sizebot
Copy link

sizebot commented Aug 17, 2021

Comparing: bd25570...130b59e

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 = 127.58 kB 127.58 kB = 40.72 kB 40.72 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.40 kB 130.40 kB = 41.65 kB 41.65 kB
facebook-www/ReactDOM-prod.classic.js = 405.16 kB 405.16 kB = 75.04 kB 75.04 kB
facebook-www/ReactDOM-prod.modern.js = 393.72 kB 393.72 kB = 73.32 kB 73.32 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.16 kB 405.16 kB = 75.05 kB 75.05 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 130b59e

@rickhanlonii
Copy link
Member

Is it conceptually possible to flow type this so we catch it if it happens again?

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 18, 2021 •
edited
Loading

Is it conceptually possible to flow type this so we catch it if it happens again?

Unclear, since (in this case) the missing properties weren't part of the statically declared type, but rather, were dynamic properties added in the __DEV__ mode build.

I'm going to merge this fix now (since the UMD bundle is broken) but we can follow up on this.

@bvaughn bvaughn merged commit 9eb2aaa into facebook:main Aug 18, 2021
@bvaughn bvaughn deleted the fix-umd-build branch August 18, 2021 05:40
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Aug 24, 2021
@kacieb @facebook-github-bot
React Native sync for revisions 424fe58...bd5bf55
7299cb4 
Summary:
Post: https://fb.workplace.com/groups/rnsyncsquad/permalink/879923262900946/

This sync includes the following changes:
- **[fc3b6a411](facebook/react@fc3b6a411 )**: Fix a few typos ([#22154](facebook/react#22154)) //<Bowen>//
- **[986d0e61d](facebook/react@986d0e61d )**: [Scheduler] Add tests for isInputPending ([#22140](facebook/react#22140)) //<Andrew Clark>//
- **[d54be90be](facebook/react@d54be90be )**: Set up test infra for dynamic Scheduler flags ([#22139](facebook/react#22139)) //<Andrew Clark>//
- **[7ed0706d7](facebook/react@7ed0706d7 )**: Remove the warning for setState on unmounted components ([#22114](facebook/react#22114)) //<Dan Abramov>//
- **[9eb2aaaf8](facebook/react@9eb2aaaf8 )**: Fixed ReactSharedInternals export in UMD bundle ([#22117](facebook/react#22117)) //<Brian Vaughn>//
- **[bd255700d](facebook/react@bd255700d )**: Show a soft error when a text string or number is supplied as a child to non text wrappers ([#22109](facebook/react#22109)) //<Sota>//

Changelog:
[General][Changed] - React Native sync for revisions 424fe58...bd5bf55

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D30485521

fbshipit-source-id: c5b92356e9e666eae94536ed31b8de43536419f8
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
@zhengjitf
Fixed ReactSharedInternals export in UMD bundle (facebook#22117)
c86c894
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaearon gaearon gaearon approved these changes

@rickhanlonii rickhanlonii rickhanlonii approved these changes

@acdlite acdlite Awaiting requested review from acdlite

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

react-dom@alpha UMD bundle throws when rendering
5 participants
@bvaughn @sizebot @rickhanlonii @gaearon @facebook-github-bot