Move modern strict to experimental

[rickhanlonii]

Turn this on

Edited: ope, nvm

Looks like there's still an outstanding issue with this. The original PR turned off a strict effects test, which causes a stray `componentWillUnmount` to fire.

5d1ce65#diff-19df471970763c4790c2cc0811fd2726cc6a891b0e1d5dedbf6d0599240c127aR70

Before:

expect(log).toEqual([
      'constructor',
      'constructor',
      'getDerivedStateFromProps',
      'getDerivedStateFromProps',
      'render',
      'render',
      'componentDidMount',
    ]);

After:

expect(log).toEqual([
      'constructor',
      'constructor',
      'getDerivedStateFromProps',
      'getDerivedStateFromProps',
      'render',
      'render',
      'componentDidMount',
      'componentWillUnmount',
      'componentDidMount',
    ]);

So there's a bug somewhere

[react-sizebot]

Comparing: 374fd68...4afefce

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	=	176.82 kB	176.82 kB	=	55.12 kB	55.12 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.81 kB	178.81 kB	=	55.70 kB	55.69 kB
facebook-www/ReactDOM-prod.classic.js	=	593.39 kB	593.39 kB	=	104.69 kB	104.69 kB
facebook-www/ReactDOM-prod.modern.js	=	577.17 kB	577.17 kB	=	101.78 kB	101.78 kB
test_utils/ReactAllWarnings.js	Deleted	67.02 kB	0.00 kB	Deleted	16.42 kB	0.00 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-art/cjs/react-art.development.js	+0.33%	857.41 kB	860.28 kB	+0.37%	187.42 kB	188.11 kB
oss-experimental/react-art/umd/react-art.development.js	+0.31%	974.49 kB	977.48 kB	+0.35%	206.57 kB	207.30 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js	+0.30%	957.60 kB	960.47 kB	+0.33%	205.96 kB	206.65 kB
oss-experimental/react-dom/cjs/react-dom.development.js	+0.21%	1,354.00 kB	1,356.87 kB	+0.23%	299.19 kB	299.88 kB
oss-experimental/react-dom/umd/react-dom.development.js	+0.21%	1,419.00 kB	1,422.00 kB	+0.24%	302.23 kB	302.95 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js	+0.21%	1,371.85 kB	1,374.72 kB	+0.23%	303.47 kB	304.17 kB
test_utils/ReactAllWarnings.js	Deleted	67.02 kB	0.00 kB	Deleted	16.42 kB	0.00 kB

Generated by 🚫 dangerJS against 4afefce

[tyao1]

which causes a stray componentWillUnmount to fire.

Noob question, I don't see a stray componentWillUnmount in after, isn't after the current behavior of StrictMode that double invoke effects?

[rickhanlonii]

oh wow @tyao1 you're right, i haven't used classes in so long i forgot what the method was called, i was expecting componentDidUnmount oops

[rickhanlonii]

Yeah I have no idea what that failure is, dev tools seems to error but I cannot figure out how to see what the error is.

@hoxyq are you familiar with this test at all or know how to debug it to see what the error is? I tried and got stuck in test abstraction hell

[hoxyq]

Yeah I have no idea what that failure is, dev tools seems to error but I cannot figure out how to see what the error is.

@hoxyq are you familiar with this test at all or know how to debug it to see what the error is? I tried and got stuck in test abstraction hell

Looking into that now

[hoxyq]

I was able to triage up to this line:

react/packages/react-devtools-shared/src/backend/console.js

Line 224 in 2477384

const current: ?Fiber = getCurrentFiber();

Let me share a bit more context whats going on here:

• React DevTools patches the console methods in order to collect errors and warnings, which will be later displayed on the components tab
• When ReactDOM.render is called in DEV, it emits the error message Warning: ReactDOM.render is no longer supported in React 18...
• Before these changes: current fiber is null, there is no work in progress
• With these changes: current fiber is not null, seems like React is still performing some work, that's why this message from ReactDOM is handled by React DevTools and registered as an error in component (root)

With modern strict and legacy render, the current is not null here, meaning that some work is being performed when ReactDOM.render is called. I don't have any useful knowledge around reconciler, so I don't really know if its expected or why this happens.

Questions:

• Are modern strict and legacy render (via ReactDOM.render) compatible? If not, then we should gate this test.

Possible reason why this happens:

• React DevTools still using unstable_act from React and not from 'internal-test-utils'. Plus this is not asynchronous.
• If you run other test storeStressTestConcurrent-test.js on your branch, it will also have some failing cases. This test uses concurrent rendering, so it definitely should be compatible with modern strict.
  • Possible solution: I've replaced all act calls with await require('internal-test-utils').act(...) and now tests are passing.

[hoxyq]

Next steps for me: I need to validate if migrating DevTools tests to using act from internal-test-utils will solve the issues.

The tricky part here is that DevTools tests are gated for a specific React version:

• If test is gated for <18 version, we should use act as require('react-dom/test-utils').act
• If test is gated for >= 18 && <$CURRENT_ON_MAIN, we should use act as require('react').unstable_act
• If test is not gated (runs on experimental), we should use act as require('internal-test-utils').act

Currently, this is only partially implemented and we don't use internal-test-utils:

react/packages/react-devtools-shared/src/__tests__/utils.js

Lines 22 to 24 in 2477384

	// Use `require('react-dom/test-utils').act` as a fallback for React 17, which can be used in integration tests for React DevTools.
	const actDOM =
	require('react').unstable_act || require('react-dom/test-utils').act;

react/packages/react-devtools-shared/src/__tests__/utils.js

Lines 49 to 51 in 2477384

	// Use `require('react-dom/test-utils').act` as a fallback for React 17, which can be used in integration tests for React DevTools.
	const actDOM =
	require('react').unstable_act || require('react-dom/test-utils').act;

Can this change wait before the end of the week, so I can try to implement this?

[rickhanlonii]

Do you have a sense for why that would work? I can't figure out what the unexpected error actually is, I can only find the error count. If I knew the error, I could tell you if that would fix it.

Based on the changes, I wouldn't expect any new errors (this is only a strict mode change, and these tests don't use strict mode) so I don't expect changing act would fix it.

[hoxyq]

Based on the changes, I wouldn't expect any new errors (this is only a strict mode change, and these tests don't use strict mode) so I don't expect changing act would fix it.

There is more to that, this flag is used in reconciler:

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines 3706 to 3728 in 45582c6

	if (__DEV__) {
	if (useModernStrictMode) {
	let doubleInvokeEffects = true;
	if (root.tag === LegacyRoot && !(root.current.mode & StrictLegacyMode)) {
	doubleInvokeEffects = false;
	}
	if (
	root.tag === ConcurrentRoot &&
	!(root.current.mode & (StrictLegacyMode | StrictEffectsMode))
	) {
	doubleInvokeEffects = false;
	}
	recursivelyTraverseAndDoubleInvokeEffectsInDEV(
	root,
	root.current,
	doubleInvokeEffects,
	);
	} else {
	legacyCommitDoubleInvokeEffectsInDEV(root.current, hasPassiveEffects);
	}
	}
	}

Its actually changes the way how current fiber is set in DEV:

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Line 3660 in 45582c6

setCurrentDebugFiberInDEV(fiber);

And this is the injected dependency from React into React DevTools (getCurrentFiber), so RDT will know which fiber has just emitted an error. For some reason, if we are using unstable_act from react, the effects invocation phase is not finished and leaves current fiber as non-null, which makes React DevTools think that error was emitted by this fiber.

Maybe there is someone who can explain the reason behind having internal version of act? This should be the answer.
cc @sammy-SC @acdlite

[hoxyq]

Okay, I think I found the issue here. React DevTools console patching works incorrectly for this scenario with modern strict mode.

When React DevTools hook is injected, it sets up the callback setStrictMode, which is used by React to notify RDT. When this callback is executed, RDT patches the console.

In this case with modern strict mode, such patching doesn't happen, that is why the error is not properly filtered.

[hoxyq]

Okay, I think I found the issue here. React DevTools console patching works incorrectly for this scenario with modern strict mode.

When React DevTools hook is injected, it sets up the callback setStrictMode, which is used by React to notify RDT. When this callback is executed, RDT patches the console.

In this case with modern strict mode, such patching doesn't happen, that is why the error is not properly filtered.

Nope, this is not the case here, unfortunately: validated that with legacy strict mode these functions are also not called.

[rickhanlonii]

@sammy-SC @acdlite, I fixed this bug that turned on strict effects in Legacy Mode, which may be the reason it was hard to land internally, breaking unit tests and whatnot.

Previously, legacyCommitDoubleInvokeEffectsInDEV would be called in the legacy root too (when strict mode was on), but we wouldn't fire any effects because no fiber flags were set. When we copied over the check to this block, it forces effects to be fired, since it's not checking fiber flags (by design).

We have a test that caught this, but we didn't have any test variants that enabled useModernStrictMode:

react/packages/react-reconciler/src/__tests__/StrictEffectsMode-test.js

Line 29 in 30e2938

it('should not double invoke effects in legacy mode', async () => {