Debug render-phase side effects in "strict" mode

[bvaughn]

Resolves #12089

A new feature flag has been added, debugRenderPhaseSideEffectsForStrictMode. When enabled, StrictMode subtrees will also double-invoke lifecycles in the same way as debugRenderPhaseSideEffects.

By default, this flag is enabled for __DEV__ only. Internally we can toggle it with a GK.

This breaks several of our incremental tests which make use of the noop-renderer. Updating the tests to account for the double-rendering in development mode makes them significantly more complicated. The most straight forward fix for this will be to convert them to be run as internal tests only. I believe this is reasonable since we are the only people making use of the noop renderer.

[bvaughn]

This change breaks the following tests:

• ReactCallReturn-test
• ReactNewContext-test.internal
• ReactIncremental-test
• ReactIncrementalErrorHandling-test
• ReactIncrementalErrorLogging-test.internal
• ReactIncrementalReflection-test
• ReactIncrementalScheduling-test
• ReactIncrementalSideEffects-test
• ReactIncrementalUpdates-test
• ReactIncrementalTriangle-test

Two of them are internal and so will be easy to fix. The rest of them are incremental tests, built with the (private) noop-renderer, and so should be okay to make internal-only. The only exception is ReactCallReturn-test which also uses the noop-renderer.

[gaearon]

The rest of them are incremental tests, built with the (private) noop-renderer, and so should be okay to make internal-only.

That seems a bit unfortunate to me. They describe the async behavior in detail and I intentionally worked to get the noop renderer to build in a way that this behavior is testable. We're undoing this work here, and I'm not sure for how long (can we remove this later?)

Technically this used to verifiy that our custom renderer infrastructure (which I changed noop renderer bundle to use) is working. Making those tests internal defies that somewhat.

I understand it's important to unblock this but I hope we can go back to running them on the bundles before we break something else and it becomes a lot of work again.

[bvaughn]

Hm. Interesting perspective, Dan.

It seems odd to me that we had tests (arguably some of our most complex tests) running against a production bundle of something that we never actually ship anywhere. And since this twice-invoking behavior adds significant complexity it felt like the right trade-off.

Technically this used to verifiy that our custom renderer infrastructure

Maybe we can achieve the same goal with tests that aren't so complex?

We're undoing this work here, and I'm not sure for how long (can we remove this later?)

I don't think this will be removed as long as the class component API is around in the form it is today.

[sebmarkbage]

Testing the noop renderer seems fine for testing the custom renderer infrastructure, but I think the async tests specifically is not the right way of doing that since they have all these special quirks that you wouldn't test for in user tests or maybe even experience. It was just accidental that we ended up only having async tests for this particular renderer.

[bvaughn]

Testing the noop renderer seems fine for testing the custom renderer infrastructure, but I think the async tests specifically is not the right way of doing

It was just accidental that we ended up only having async tests for this particular renderer.

Right, because noop renderer is always async:

react/packages/react-noop-renderer/src/ReactNoop.js

Line 345 in d3b183c

root = NoopRenderer.createContainer(container, true, false);

react/packages/react-noop-renderer/src/ReactNoop.js

Line 365 in d3b183c

root = PersistentNoopRenderer.createContainer(container, true, false);

So any tests for it would have to account for the double lifecycles in DEV. Granted, some of them are pretty straight forward though.

[gaearon]

I think the confusing part here is that testing custom renderer infra and testing async are two unrelated activities but they happen to be combined here.

I think both are valuable to do against the bundles:

• Testing custom renderers is important so we don’t regress there with silly mistakes.

• Testing async is important because normal ReactDOM tests don’t cover a lot of behaviors in the reconciler. If we move forward with more sophisticated compilation (or even if Rollup or GCC has a bug) we won’t catch anything that’s not in the synchronous path unless we run those tests on bundles.

[bvaughn]

Agreed.