Enable the StrictEffects flag for OSS builds

[gaearon]

Seems like an omission. Are there more we should enable? Also, is it ok to land this to main now, or do we still plan to cut minors from main?

TODO:

• Fix tests (why do they double-call with render?)
• Figure out the DEV thing
• What about enableSuspenseLayoutEffectSemantics?

[bvaughn]

Suggested change

	export const enableStrictEffects = true;
	export const enableStrictEffects = __DEV__;

[gaearon]

I could do this but there's a small inconsistency in that the WWW version re-exports a GK which does not have a DEV check. I'd rather have them all consistent. Currently I think we always check for __DEV__ in the source anyway before any behavioral changes. So I'd propose either:

1. Keep it true
2. Make it __DEV__ and change WWW feature flag to be __DEV__ && FeatureFlagsWWW.enableStrictEffects

[bvaughn]

I think it makes more sense to compare to other flags within this feature flags file rather than www. So this would be more like debugRenderPhaseSideEffectsForStrictMode right above it.

[bvaughn]

Currently I think we always check for __DEV__ in the source anyway before any behavioral changes.

Yeah, but this is to enable static dead code elimination in production builds even if there's a dynamic flag (like in www). I think the feature flags still generally use __DEV__ or __PROFILING__ to be explicit. :)

[gaearon]

I think it makes more sense to compare to other flags within this feature flags file rather than www.

I'm thinking of a situation where we add some behavior difference in the source without checking __DEV__ before it (by mistake). Then, if WWW and OSS flags diverge in this way, you'd see the different behavior in production on WWW than in production in OSS. That seems like a risky divergence.

[bvaughn]

Then let's change the OSS www export too.

There's good reason to keep the extra static __DEV__ wrapper (mentioned above) and I think there's also value in keeping the feature flags more scoped/explicit about DEV-only features.

[bvaughn]

Whoops.

[sizebot]

Comparing: d75105f...187567b

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	=	126.00 kB	126.00 kB	=	40.41 kB	40.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	128.82 kB	128.82 kB	=	41.35 kB	41.35 kB
facebook-www/ReactDOM-prod.classic.js	=	406.09 kB	406.09 kB	=	75.11 kB	75.11 kB
facebook-www/ReactDOM-prod.modern.js	=	394.46 kB	394.46 kB	=	73.29 kB	73.29 kB
facebook-www/ReactDOMForked-prod.classic.js	=	406.09 kB	406.09 kB	=	75.11 kB	75.11 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-art/cjs/react-art.development.js	+1.07%	622.18 kB	628.82 kB	+0.76%	135.58 kB	136.62 kB
oss-experimental/react-art/cjs/react-art.development.js	+1.03%	644.64 kB	651.29 kB	+0.74%	140.25 kB	141.29 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js	+1.00%	681.34 kB	688.14 kB	+0.70%	145.47 kB	146.49 kB
oss-stable/react-art/umd/react-art.development.js	+0.97%	723.53 kB	730.54 kB	+0.68%	153.78 kB	154.83 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js	+0.97%	704.00 kB	710.80 kB	+0.67%	150.10 kB	151.10 kB
oss-experimental/react-art/umd/react-art.development.js	+0.94%	747.35 kB	754.36 kB	+0.65%	158.53 kB	159.56 kB
oss-stable/react-dom/umd/react-dom.development.js	+0.73%	987.06 kB	994.23 kB	+0.48%	215.25 kB	216.28 kB
oss-stable/react-dom/cjs/react-dom.development.js	+0.72%	939.84 kB	946.64 kB	+0.48%	212.70 kB	213.71 kB
oss-experimental/react-dom/umd/react-dom.development.js	+0.71%	1,012.96 kB	1,020.13 kB	+0.47%	220.26 kB	221.28 kB
oss-experimental/react-dom/cjs/react-dom.development.js	+0.71%	964.27 kB	971.07 kB	+0.47%	217.69 kB	218.71 kB

Generated by 🚫 dangerJS against 187567b

[gaearon]

This failure is curious:

 FAIL  packages/react/src/__tests__/ReactStrictMode-test.js
  ● ReactStrictMode › should invoke precommit lifecycle methods twice

    expect(received).toEqual(expected) // deep equality

    - Expected  - 0
    + Received  + 2

    @@ -4,6 +4,8 @@
        "getDerivedStateFromProps",
        "getDerivedStateFromProps",
        "render",
        "render",
        "componentDidMount",
    +   "componentWillUnmount",
    +   "componentDidMount",
      ]

      105 |
      106 |     if (__DEV__) {
    > 107 |       expect(log).toEqual([
          |                   ^
      108 |         'constructor',
      109 |         'constructor',
      110 |         'getDerivedStateFromProps',

That test has ReactDOM.render, not createRoot. Why does it fail? I'll look into this later.

[bvaughn]

PR #20639 landed "strict effects" for new root API only, but when #20849 introduced strict mode levels, it looks like the new root check got changed the check from

(workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode

to

(workInProgress.mode & StrictEffectsMode) !== NoMode

I don't remember if this was intentional, although maybe it was since the new level required an explicit opt-in (and wouldn't be rolled out to pre-existing strict trees)?

[bvaughn]

#21591 should correct the above issue and fix the failing tests in this branch.

[bvaughn]

Now that #21591 has landed, we should be good to rebase and land this one!

[bvaughn]

Since you're off today I'm going to go ahead and take up the two action items left over from this PR with #21597