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

Add default for current act queue #21935

Closed

Conversation

rickhanlonii
Copy link
Member

Overview

If you're depending on an older version of React than the renderer uses, we'll crash on ReactCurrentActQueue is undefined.

For example, OSS React Native builds use the sync'd renderer but the latest OSS React version. This is a simple fix to default ReactCurrentActQueue so it doesn't error. Another fix would be to feature flag this to non-React Native OSS builds, but that seem heavy handed.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 21, 2021
@acdlite
Copy link
Collaborator

acdlite commented Jul 21, 2021

OSS React Native builds use the sync'd renderer but the latest OSS React version

We need to fix that

@rickhanlonii
Copy link
Member Author

So we should just not sync those files when we run the React Native sync, and only build them when they upgrade the OSS version of React?

@sizebot
Copy link

sizebot commented Jul 21, 2021

Comparing: 9b76d2d...1ac5ca2

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.50 kB 127.50 kB = 40.71 kB 40.71 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 130.32 kB 130.32 kB = 41.66 kB 41.66 kB
facebook-www/ReactDOM-prod.classic.js = 405.88 kB 405.88 kB = 74.97 kB 74.97 kB
facebook-www/ReactDOM-prod.modern.js = 394.58 kB 394.58 kB = 73.25 kB 73.25 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.88 kB 405.88 kB = 74.97 kB 74.97 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 1ac5ca2

@acdlite
Copy link
Collaborator

acdlite commented Jul 21, 2021

Historically we've tried to maintain backwards/forwards compatibility between the isomorphic package and the renderer within the same major version. (I think we should consider being even stricter in the future, but set that aside for now).

The open source version is React 17, whereas main is on React 18. So I don't think we should be adding extra code to maintain backwards compatibility with a 17 build.

Another fix would be to feature flag this to non-React Native OSS builds, but that seem heavy handed.

I find it less heavy handed than allowing this to leak into the open source build, though

@acdlite
Copy link
Collaborator

acdlite commented Jul 21, 2021

So we should just not sync those files when we run the React Native sync, and only build them when they upgrade the OSS version of React?

We should use the synced isomorphic package and the synced renderer, instead of mismatching them. They're separate packages but they should be used in lockstep.

@rickhanlonii
Copy link
Member Author

rickhanlonii commented Jul 21, 2021

We should use the synced isomorphic package and the synced renderer

We do that internally, this is for OSS builds. If this were feature flagged, it would be on in ReactFeatureFlags.native-fb.js and off in ReactFeatureFlags.native-oss.js. But thinking about it more, we should probably only sync updates to the OSS renderer files when we actually upgrade the OSS react version.

@acdlite
Copy link
Collaborator

acdlite commented Jul 21, 2021

The open source version is React 17, whereas main is on React 18. So I don't think we should be adding extra code to maintain backwards compatibility with a 17 build.

A counter-argument is that 17 is designed to allow for multiple React versions in the same app, but our recommendation there is to use separate Webpack configs to bundle multiple Reacts. Otherwise we're setting a precedent that the isomorphic package has to be perpetually compatible, and I'd really rather not do that.

@acdlite
Copy link
Collaborator

acdlite commented Jul 21, 2021

We do that internally, this is for OSS builds. If this were feature flagged, it would be on in ReactFeatureFlags.native-fb.js and off in ReactFeatureFlags.native-oss.js. But thinking about it more, we should probably only sync updates to the OSS renderer files when we actually upgrade the OSS react version.

I don't really get how RN open source releases work, but the principle is that there shouldn't be a mismatch between the isomorphic package and the renderer. So if you sync one, you should sync the other.

@rickhanlonii
Copy link
Member Author

Yes, the issue is that we're syncing the OSS renderer but not upgrading the OSS version of React, because it hasn't been published yet. I'll fix in the sync script.

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.

None yet

4 participants