Backed out DevTools feature flag fork in favor of global env vars

facebook · Apr 17, 2020 · 51ce8bc · 51ce8bc
1 parent e73fb41
commit 51ce8bc
Show file tree

Hide file tree

Showing 10 changed files with 47 additions and 109 deletions.
diff --git a/packages/react-devtools-core/webpack.backend.js b/packages/react-devtools-core/webpack.backend.js
@@ -38,12 +38,13 @@ module.exports = {
       'react-debug-tools': resolve(builtModulesDir, 'react-debug-tools'),
       'react-is': resolve(builtModulesDir, 'react-is'),
       scheduler: resolve(builtModulesDir, 'scheduler'),
-      'shared/ReactFeatureFlags': 'shared/forks/ReactFeatureFlags.devtools',
     },
   },
   plugins: [
     new DefinePlugin({
       __DEV__: true,
+      __PROFILE__: false,
+      __EXPERIMENTAL__: true,
       'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
       'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
     }),

diff --git a/packages/react-devtools-core/webpack.standalone.js b/packages/react-devtools-core/webpack.standalone.js
@@ -37,7 +37,6 @@ module.exports = {
       'react-debug-tools': resolve(builtModulesDir, 'react-debug-tools'),
       'react-is': resolve(builtModulesDir, 'react-is'),
       scheduler: resolve(builtModulesDir, 'scheduler'),
-      'shared/ReactFeatureFlags': 'shared/forks/ReactFeatureFlags.devtools',
     },
   },
   node: {
@@ -49,6 +48,8 @@ module.exports = {
   plugins: [
     new DefinePlugin({
       __DEV__: false,
+      __PROFILE__: false,
+      __EXPERIMENTAL__: true,
       'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
       'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
       'process.env.NODE_ENV': `"${NODE_ENV}"`,

diff --git a/packages/react-devtools-extensions/webpack.backend.js b/packages/react-devtools-extensions/webpack.backend.js
@@ -33,12 +33,13 @@ module.exports = {
       'react-dom': resolve(builtModulesDir, 'react-dom'),
       'react-is': resolve(builtModulesDir, 'react-is'),
       scheduler: resolve(builtModulesDir, 'scheduler'),
-      'shared/ReactFeatureFlags': 'shared/forks/ReactFeatureFlags.devtools',
     },
   },
   plugins: [
     new DefinePlugin({
       __DEV__: true,
+      __PROFILE__: false,
+      __EXPERIMENTAL__: true,
       'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
       'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
     }),

diff --git a/packages/react-devtools-extensions/webpack.config.js b/packages/react-devtools-extensions/webpack.config.js
@@ -38,12 +38,13 @@ module.exports = {
       'react-dom': resolve(builtModulesDir, 'react-dom'),
       'react-is': resolve(builtModulesDir, 'react-is'),
       scheduler: resolve(builtModulesDir, 'scheduler'),
-      'shared/ReactFeatureFlags': 'shared/forks/ReactFeatureFlags.devtools',
     },
   },
   plugins: [
     new DefinePlugin({
       __DEV__: false,
+      __PROFILE__: false,
+      __EXPERIMENTAL__: true,
       'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
       'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
       'process.env.NODE_ENV': `"${NODE_ENV}"`,

diff --git a/packages/react-devtools-inline/webpack.config.js b/packages/react-devtools-inline/webpack.config.js
@@ -35,11 +35,12 @@ module.exports = {
     'react-dom': 'react-dom',
     'react-is': 'react-is',
     scheduler: 'scheduler',
-    'shared/ReactFeatureFlags': 'shared/forks/ReactFeatureFlags.devtools',
   },
   plugins: [
     new DefinePlugin({
       __DEV__,
+      __PROFILE__: false,
+      __EXPERIMENTAL__: true,
       'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
       'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
       'process.env.NODE_ENV': `"${NODE_ENV}"`,

diff --git a/packages/react-devtools-shared/src/__tests__/console-test.js b/packages/react-devtools-shared/src/__tests__/console-test.js
@@ -114,7 +114,7 @@ describe('console', () => {
   });
 
   it('should not append multiple stacks', () => {
-    const Child = () => {
+    const Child = ({children}) => {
       fakeConsole.warn('warn\n    in Child (at fake.js:123)');
       fakeConsole.error('error', '\n    in Child (at fake.js:123)');
       return null;
@@ -135,12 +135,12 @@ describe('console', () => {
 
   it('should append component stacks to errors and warnings logged during render', () => {
     const Intermediate = ({children}) => children;
-    const Parent = () => (
+    const Parent = ({children}) => (
       <Intermediate>
         <Child />
       </Intermediate>
     );
-    const Child = () => {
+    const Child = ({children}) => {
       fakeConsole.error('error');
       fakeConsole.log('log');
       fakeConsole.warn('warn');
@@ -149,42 +149,31 @@ describe('console', () => {
 
     act(() => ReactDOM.render(<Parent />, document.createElement('div')));
 
-    // TRICKY DevTools console override re-renders the component to intentionally trigger an error,
-    // in which case all of the mock functions will be called an additional time.
-
-    expect(mockError).toHaveBeenCalledTimes(2);
-    expect(mockError.mock.calls[0]).toHaveLength(1);
-    expect(mockError.mock.calls[0][0]).toBe('error');
-    expect(mockError.mock.calls[1]).toHaveLength(2);
-    expect(mockError.mock.calls[1][0]).toBe('error');
-    expect(normalizeCodeLocInfo(mockError.mock.calls[1][1])).toBe(
-      '\n    in Child (at **)\n    in Intermediate (at **)\n    in Parent (at **)',
-    );
-
-    expect(mockLog).toHaveBeenCalledTimes(2);
+    expect(mockLog).toHaveBeenCalledTimes(1);
     expect(mockLog.mock.calls[0]).toHaveLength(1);
     expect(mockLog.mock.calls[0][0]).toBe('log');
-    expect(mockLog.mock.calls[1]).toHaveLength(1);
-    expect(mockLog.mock.calls[1][0]).toBe('log');
-
-    expect(mockWarn).toHaveBeenCalledTimes(2);
-    expect(mockWarn.mock.calls[0]).toHaveLength(1);
+    expect(mockWarn).toHaveBeenCalledTimes(1);
+    expect(mockWarn.mock.calls[0]).toHaveLength(2);
     expect(mockWarn.mock.calls[0][0]).toBe('warn');
-    expect(mockWarn.mock.calls[1]).toHaveLength(2);
-    expect(mockWarn.mock.calls[1][0]).toBe('warn');
-    expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][1])).toEqual(
+    expect(normalizeCodeLocInfo(mockWarn.mock.calls[0][1])).toEqual(
+      '\n    in Child (at **)\n    in Intermediate (at **)\n    in Parent (at **)',
+    );
+    expect(mockError).toHaveBeenCalledTimes(1);
+    expect(mockError.mock.calls[0]).toHaveLength(2);
+    expect(mockError.mock.calls[0][0]).toBe('error');
+    expect(normalizeCodeLocInfo(mockError.mock.calls[0][1])).toBe(
       '\n    in Child (at **)\n    in Intermediate (at **)\n    in Parent (at **)',
     );
   });
 
   it('should append component stacks to errors and warnings logged from effects', () => {
     const Intermediate = ({children}) => children;
-    const Parent = () => (
+    const Parent = ({children}) => (
       <Intermediate>
         <Child />
       </Intermediate>
     );
-    const Child = () => {
+    const Child = ({children}) => {
       React.useLayoutEffect(() => {
         fakeConsole.error('active error');
         fakeConsole.log('active log');
@@ -231,7 +220,7 @@ describe('console', () => {
 
   it('should append component stacks to errors and warnings logged from commit hooks', () => {
     const Intermediate = ({children}) => children;
-    const Parent = () => (
+    const Parent = ({children}) => (
       <Intermediate>
         <Child />
       </Intermediate>
@@ -287,7 +276,7 @@ describe('console', () => {
 
   it('should append component stacks to errors and warnings logged from gDSFP', () => {
     const Intermediate = ({children}) => children;
-    const Parent = () => (
+    const Parent = ({children}) => (
       <Intermediate>
         <Child />
       </Intermediate>
@@ -325,7 +314,7 @@ describe('console', () => {
   });
 
   it('should append stacks after being uninstalled and reinstalled', () => {
-    const Child = () => {
+    const Child = ({children}) => {
       fakeConsole.warn('warn');
       fakeConsole.error('error');
       return null;
@@ -344,20 +333,17 @@ describe('console', () => {
     patchConsole();
     act(() => ReactDOM.render(<Child />, document.createElement('div')));
 
-    // TRICKY DevTools console override re-renders the component to intentionally trigger an error,
-    // in which case all of the mock functions will be called an additional time.
-
-    expect(mockWarn).toHaveBeenCalledTimes(3);
-    expect(mockWarn.mock.calls[2]).toHaveLength(2);
-    expect(mockWarn.mock.calls[2][0]).toBe('warn');
-    expect(normalizeCodeLocInfo(mockWarn.mock.calls[2][1])).toEqual(
+    expect(mockWarn).toHaveBeenCalledTimes(2);
+    expect(mockWarn.mock.calls[1]).toHaveLength(2);
+    expect(mockWarn.mock.calls[1][0]).toBe('warn');
+    expect(normalizeCodeLocInfo(mockWarn.mock.calls[1][1])).toEqual(
       '\n    in Child (at **)',
     );
-    expect(mockError).toHaveBeenCalledTimes(3);
-    expect(mockError.mock.calls[2]).toHaveLength(2);
-    expect(mockError.mock.calls[2][0]).toBe('error');
-    expect(normalizeCodeLocInfo(mockError.mock.calls[2][1])).toBe(
+    expect(mockError).toHaveBeenCalledTimes(2);
+    expect(mockError.mock.calls[1]).toHaveLength(2);
+    expect(mockError.mock.calls[1][0]).toBe('error');
+    expect(normalizeCodeLocInfo(mockError.mock.calls[1][1])).toBe(
       '\n    in Child (at **)',
     );
   });
-});
+});
diff --git a/packages/react-devtools-shared/src/backend/console.js b/packages/react-devtools-shared/src/backend/console.js
@@ -107,7 +107,10 @@ export function patch(): void {
             for (const {getCurrentFiber} of injectedRenderers.values()) {
               const current: ?Fiber = getCurrentFiber();
               if (current != null) {
-                args.push(getStackByFiberInDevAndProd(current));
+                const componentStack = getStackByFiberInDevAndProd(current);
+                if (componentStack !== '') {
+                  args.push(componentStack);
+                }
                 break;
               }
             }

diff --git a/packages/react-devtools-shell/webpack.config.js b/packages/react-devtools-shell/webpack.config.js
@@ -37,12 +37,13 @@ const config = {
       'react-debug-tools': resolve(builtModulesDir, 'react-debug-tools'),
       'react-is': resolve(builtModulesDir, 'react-is'),
       scheduler: resolve(builtModulesDir, 'scheduler'),
-      'shared/ReactFeatureFlags': 'shared/forks/ReactFeatureFlags.devtools',
     },
   },
   plugins: [
     new DefinePlugin({
       __DEV__,
+      __PROFILE__: false,
+      __EXPERIMENTAL__: true,
       'process.env.GITHUB_URL': `"${GITHUB_URL}"`,
       'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`,
     }),

diff --git a/packages/shared/forks/ReactFeatureFlags.devtools.js b/packages/shared/forks/ReactFeatureFlags.devtools.js
diff --git a/scripts/jest/config.build-devtools.js b/scripts/jest/config.build-devtools.js
@@ -26,13 +26,6 @@ const packages = readdirSync(packagesRoot).filter(dir => {
 // Create a module map to point React packages to the build output
 const moduleNameMapper = {};
 
-// Allow bundle tests to read (but not write!) default feature flags.
-// This lets us determine whether we're running in different modes
-// without making relevant tests internal-only.
-moduleNameMapper[
-  '^shared/ReactFeatureFlags'
-] = `<rootDir>/packages/shared/forks/ReactFeatureFlags.readonly`;
-
 // Map packages to bundles
 packages.forEach(name => {
   // Root entry point
@@ -43,6 +36,10 @@ packages.forEach(name => {
   ] = `<rootDir>/build/node_modules/${name}/$1`;
 });
 
+// Allow tests to import shared code (e.g. feature flags, getStackByFiberInDevAndProd)
+moduleNameMapper['^shared\/([^\/]+)$'] = '<rootDir>/packages/shared/$1';
+moduleNameMapper['^react-reconciler\/([^\/]+)$'] = '<rootDir>/packages/react-reconciler/$1';
+
 module.exports = Object.assign({}, baseConfig, {
   // Redirect imports to the compiled bundles
   moduleNameMapper,