Adjust consoleManagedByDevToolsDuringStrictMode feature flag #22253

gaearon · 2021-09-06T18:43:08Z

See individual comments for justification.

gaearon

this is what I would expect, would appreciate help on what I'm misunderstanding :)

gaearon · 2021-09-06T18:43:34Z

packages/shared/ReactFeatureFlags.js

@@ -176,4 +176,4 @@ export const allowConcurrentByDefault = false;

 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = false;
+export const consoleManagedByDevToolsDuringStrictMode = true;


I'd expect this to be true because it's OSS web React, which is managed by DevTools

This was an oversight from the previous PR. Thanks for catching.

gaearon · 2021-09-06T18:43:49Z

packages/shared/forks/ReactFeatureFlags.test-renderer.js

@@ -61,7 +61,7 @@ export const enableSyncDefaultUpdates = true;
 export const allowConcurrentByDefault = false;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = false;


I'd expect this to be false because test renderer doesn't talk to DevTools

This (and other test flag changes below) seems reasonable.

gaearon · 2021-09-06T18:43:53Z

packages/shared/forks/ReactFeatureFlags.test-renderer.native.js

@@ -61,7 +61,7 @@ export const enableSyncDefaultUpdates = true;
 export const allowConcurrentByDefault = true;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = false;


I'd expect this to be false because test renderer doesn't talk to DevTools

gaearon · 2021-09-06T18:43:58Z

packages/shared/forks/ReactFeatureFlags.test-renderer.www.js

@@ -61,7 +61,7 @@ export const enableSyncDefaultUpdates = true;
 export const allowConcurrentByDefault = true;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = false;


I'd expect this to be false because test renderer doesn't talk to DevTools

gaearon · 2021-09-06T18:44:29Z

packages/shared/forks/ReactFeatureFlags.testing.js

@@ -61,7 +61,7 @@ export const enableSyncDefaultUpdates = true;
 export const allowConcurrentByDefault = false;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = false;


I'd expect this to be false because the "testing" builds of ReactDOM (which we currently don't actually use) would run in an environment that probably doesn't have DevTools, so it's not clear to me that this makes sense there

gaearon · 2021-09-06T18:44:33Z

packages/shared/forks/ReactFeatureFlags.testing.www.js

@@ -61,7 +61,7 @@ export const enableSyncDefaultUpdates = true;
 export const allowConcurrentByDefault = true;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = false;


I'd expect this to be false because the "testing" builds of ReactDOM (which we currently don't actually use) would run in an environment that probably doesn't have DevTools, so it's not clear to me that this makes sense there

gaearon · 2021-09-06T18:45:03Z

packages/shared/forks/ReactFeatureFlags.www-dynamic.js

@@ -60,4 +60,4 @@ export const enableSyncDefaultUpdates = __VARIANT__;
 export const allowConcurrentByDefault = true;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = __VARIANT__;


The instructions in this file tell me to use the VARIANT configuration instead of hardcoding a value so I assume that's what I should do

I don't have a strong opinion about this change one way or another. We have several other hard-coded values in this file, FWIW.

If I understand correctly those were originally TODOs that failed when this file was introduced. At the time the assumption was that we’d convert them all, and that any newly added ones should be added above (and use VARIANT). Maybe we should make that comment clearer.

bvaughn · 2021-09-06T20:59:49Z

packages/shared/forks/ReactFeatureFlags.www-dynamic.js

@@ -60,4 +60,4 @@ export const enableSyncDefaultUpdates = __VARIANT__;
 export const allowConcurrentByDefault = true;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = __VARIANT__;


I don't have a strong opinion about this change one way or another. We have several other hard-coded values in this file, FWIW.

bvaughn · 2021-09-06T21:00:58Z

packages/shared/ReactFeatureFlags.js

@@ -176,4 +176,4 @@ export const allowConcurrentByDefault = false;

 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = false;
+export const consoleManagedByDevToolsDuringStrictMode = true;


This was an oversight from the previous PR. Thanks for catching.

bvaughn · 2021-09-06T21:01:13Z

packages/shared/forks/ReactFeatureFlags.test-renderer.js

@@ -61,7 +61,7 @@ export const enableSyncDefaultUpdates = true;
 export const allowConcurrentByDefault = false;
 export const enablePersistentOffscreenHostContainer = false;

-export const consoleManagedByDevToolsDuringStrictMode = true;
+export const consoleManagedByDevToolsDuringStrictMode = false;


This (and other test flag changes below) seems reasonable.

sizebot · 2021-09-07T18:37:26Z

Comparing: 1314299...d6b3221

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.60 kB	127.60 kB	=	40.73 kB	40.73 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	130.42 kB	130.42 kB	=	41.66 kB	41.66 kB
facebook-www/ReactDOM-prod.classic.js	=	405.18 kB	405.18 kB	=	75.05 kB	75.05 kB
facebook-www/ReactDOM-prod.modern.js	=	393.75 kB	393.75 kB	=	73.33 kB	73.33 kB
facebook-www/ReactDOMForked-prod.classic.js	=	405.18 kB	405.18 kB	=	75.05 kB	75.05 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d6b3221

ryota-murakami · 2021-09-07T19:00:38Z

Neat, I like this naming, too explicitly what does option doing actually!

Summary: This sync includes the following changes: - **[95d762e40](facebook/react@95d762e40 )**: Remove duplicate test //<Andrew Clark>// - **[d4d1dc085](facebook/react@d4d1dc085 )**: Reorder VARIANT feature flags ([#22266](facebook/react#22266)) //<Dan Abramov>// - **[2f156eafb](facebook/react@2f156eafb )**: Adjust consoleManagedByDevToolsDuringStrictMode feature flag ([#22253](facebook/react#22253)) //<Dan Abramov>// - **[cfd819332](facebook/react@cfd819332 )**: Add useSyncExternalStore to react-debug-tools ([#22240](facebook/react#22240)) //<Andrew Clark>// - **[8e80592a3](facebook/react@8e80592a3 )**: Remove state queue from useSyncExternalStore ([#22265](facebook/react#22265)) //<Andrew Clark>// - **[06f98c168](facebook/react@06f98c168 )**: Implement useSyncExternalStore in Fiber ([#22239](facebook/react#22239)) //<Andrew Clark>// - **[77912d9a0](facebook/react@77912d9a0 )**: Wire up the native API for useSyncExternalStore ([#22237](facebook/react#22237)) //<Andrew Clark>// - **[031abd24b](facebook/react@031abd24b )**: Add warning and test for useSyncExternalStore when getSnapshot isn't cached ([#22262](facebook/react#22262)) //<salazarm>// - **[b8884de24](facebook/react@b8884de24 )**: break up import keyword to avoid being accidentally parsed as dynamic import statement in external code ([#21918](facebook/react#21918)) //<Jianhua Zheng>// - **[6d6bba5bf](facebook/react@6d6bba5bf )**: Fix typo in ReactUpdatePriority-test.js ([#21958](facebook/react#21958)) //<Ikko Ashimine>// - **[0c0d1ddae](facebook/react@0c0d1ddae )**: feat(eslint-plugin-react-hooks): support ESLint 8.x ([#22248](facebook/react#22248)) //<Michaël De Boey>// - **[1314299c7](facebook/react@1314299c7 )**: Initial shim of useSyncExternalStore ([#22211](facebook/react#22211)) //<Andrew Clark>// - **[fc40f02ad](facebook/react@fc40f02ad )**: Add consoleManagedByDevToolsDuringStrictMode feature flag in React Reconciler ([#22196](facebook/react#22196)) //<Luna Ruan>// - **[46a0f050a](facebook/react@46a0f050a )**: Set up use-sync-external-store package ([#22202](facebook/react#22202)) //<Andrew Clark>// - **[8723e772b](facebook/react@8723e772b )**: Fix a string interpolation typo in ReactHooks test ([#22174](facebook/react#22174)) //<Matt Hargett>// - **[60a30cf32](facebook/react@60a30cf32 )**: Console Logging for StrictMode Double Rendering ([#22030](facebook/react#22030)) //<Luna Ruan>// - **[76bbad3e3](facebook/react@76bbad3e3 )**: Add maxYieldMs feature flag in Scheduler ([#22165](facebook/react#22165)) //<Ricky>// - **[b0b53ae2c](facebook/react@b0b53ae2c )**: Add feature flags for scheduler experiments ([#22105](facebook/react#22105)) //<Ricky>// Changelog: [General][Changed] - React Native sync for revisions bd5bf55...95d762e jest_e2e[run_all_tests] Reviewed By: mdvacca Differential Revision: D30809906 fbshipit-source-id: 131cfdf91e15f67fa59a5d925467e538ee89fe10

…k#22253)

Adjust consoleManagedByDevToolsDuringStrictMode feature flag

d6b3221

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 6, 2021

gaearon commented Sep 6, 2021

View reviewed changes

gaearon mentioned this pull request Sep 6, 2021

Add consoleManagedByDevToolsDuringStrictMode feature flag in React Reconciler #22196

Merged

bvaughn approved these changes Sep 6, 2021

View reviewed changes

gaearon merged commit 2f156ea into facebook:main Sep 7, 2021

zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022

Adjust consoleManagedByDevToolsDuringStrictMode feature flag (faceboo…

55c964e

…k#22253)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust consoleManagedByDevToolsDuringStrictMode feature flag #22253

Adjust consoleManagedByDevToolsDuringStrictMode feature flag #22253

gaearon commented Sep 6, 2021 •

edited

Loading

gaearon left a comment

gaearon Sep 6, 2021

bvaughn Sep 6, 2021

gaearon Sep 6, 2021

bvaughn Sep 6, 2021

gaearon Sep 6, 2021

gaearon Sep 6, 2021

gaearon Sep 6, 2021

gaearon Sep 6, 2021

gaearon Sep 6, 2021

bvaughn Sep 6, 2021

gaearon Sep 6, 2021

bvaughn Sep 6, 2021

bvaughn Sep 6, 2021

bvaughn Sep 6, 2021

sizebot commented Sep 7, 2021

ryota-murakami commented Sep 7, 2021

Adjust consoleManagedByDevToolsDuringStrictMode feature flag #22253

Adjust consoleManagedByDevToolsDuringStrictMode feature flag #22253

Conversation

gaearon commented Sep 6, 2021 • edited Loading

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sizebot commented Sep 7, 2021

Critical size changes

Significant size changes

ryota-murakami commented Sep 7, 2021

gaearon commented Sep 6, 2021 •

edited

Loading