From 889a8de224962ee37a35b47e171853e3c5742b1b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 9 Sep 2018 08:56:59 -0700 Subject: [PATCH 1/6] schedule/tracking (production bundle) should not cause react-dom (profiling bundle) to error --- packages/schedule/src/Tracking.js | 17 ++++++----------- .../src/__tests__/Tracking-test.internal.js | 6 ------ .../schedule/src/__tests__/Tracking-test.js | 11 +++++++++++ 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/schedule/src/Tracking.js b/packages/schedule/src/Tracking.js index 7356445e74e..050cc9be421 100644 --- a/packages/schedule/src/Tracking.js +++ b/packages/schedule/src/Tracking.js @@ -65,19 +65,14 @@ let threadIDCounter: number = 0; // Interactions "stack"– // Meaning that newly tracked interactions are appended to the previously active set. // When an interaction goes out of scope, the previous set (if any) is restored. -let interactionsRef: InteractionsRef = (null: any); +const interactionsRef: InteractionsRef = { + current: new Set(), +}; // Listener(s) to notify when interactions begin and end. -let subscriberRef: SubscriberRef = (null: any); - -if (enableSchedulerTracking) { - interactionsRef = { - current: new Set(), - }; - subscriberRef = { - current: null, - }; -} +const subscriberRef: SubscriberRef = { + current: null, +}; export {interactionsRef as __interactionsRef, subscriberRef as __subscriberRef}; diff --git a/packages/schedule/src/__tests__/Tracking-test.internal.js b/packages/schedule/src/__tests__/Tracking-test.internal.js index 8de0cbc4858..ce4b1973b4d 100644 --- a/packages/schedule/src/__tests__/Tracking-test.internal.js +++ b/packages/schedule/src/__tests__/Tracking-test.internal.js @@ -365,11 +365,5 @@ describe('Tracking', () => { wrappedCallback(); }); - - describe('advanced integration', () => { - it('should not create unnecessary objects', () => { - expect(SchedulerTracking.__interactionsRef).toBe(null); - }); - }); }); }); diff --git a/packages/schedule/src/__tests__/Tracking-test.js b/packages/schedule/src/__tests__/Tracking-test.js index 6b340f8434e..2adecf1e3f7 100644 --- a/packages/schedule/src/__tests__/Tracking-test.js +++ b/packages/schedule/src/__tests__/Tracking-test.js @@ -48,4 +48,15 @@ describe('Tracking', () => { wrappedCallback(); }); + + // Make sure that accidental pairing of react-dom/profiling production bundle, + // With production (non-profiling) schedule/tracking doesn't cause runtime error. + it('should always export non-null refs to avoid breaking react-dom/profiling bundle', () => { + expect(SchedulerTracking.__interactionsRef).toEqual({ + current: new Set(), + }); + expect(SchedulerTracking.__subscriberRef).toEqual({ + current: null, + }); + }); }); From 996eb69d0728c1b5b45bd786bab2e7d74412e330 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 9 Sep 2018 09:18:15 -0700 Subject: [PATCH 2/6] Added blessed production+profiling entry point for schedule/tracking --- packages/schedule/npm/tracking-profiling.js | 7 +++++++ packages/schedule/package.json | 1 + 2 files changed, 8 insertions(+) create mode 100644 packages/schedule/npm/tracking-profiling.js diff --git a/packages/schedule/npm/tracking-profiling.js b/packages/schedule/npm/tracking-profiling.js new file mode 100644 index 00000000000..c90851e796d --- /dev/null +++ b/packages/schedule/npm/tracking-profiling.js @@ -0,0 +1,7 @@ +'use strict'; + +if (process.env.NODE_ENV === 'production') { + module.exports = require('./cjs/schedule-tracking.profiling.min.js'); +} else { + module.exports = require('./cjs/schedule-tracking.development.js'); +} diff --git a/packages/schedule/package.json b/packages/schedule/package.json index db052a1edc9..f0ce6c2a4c5 100644 --- a/packages/schedule/package.json +++ b/packages/schedule/package.json @@ -20,6 +20,7 @@ "README.md", "index.js", "tracking.js", + "tracking-profiling.js", "cjs/", "umd/" ] From 16bef4caac089b0d28dbab85d41cbfc4244e426c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 9 Sep 2018 09:18:23 -0700 Subject: [PATCH 3/6] Revert "schedule/tracking (production bundle) should not cause react-dom (profiling bundle) to error" This reverts commit 889a8de224962ee37a35b47e171853e3c5742b1b. --- packages/schedule/src/Tracking.js | 17 +++++++++++------ .../src/__tests__/Tracking-test.internal.js | 6 ++++++ .../schedule/src/__tests__/Tracking-test.js | 11 ----------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/schedule/src/Tracking.js b/packages/schedule/src/Tracking.js index 050cc9be421..7356445e74e 100644 --- a/packages/schedule/src/Tracking.js +++ b/packages/schedule/src/Tracking.js @@ -65,14 +65,19 @@ let threadIDCounter: number = 0; // Interactions "stack"– // Meaning that newly tracked interactions are appended to the previously active set. // When an interaction goes out of scope, the previous set (if any) is restored. -const interactionsRef: InteractionsRef = { - current: new Set(), -}; +let interactionsRef: InteractionsRef = (null: any); // Listener(s) to notify when interactions begin and end. -const subscriberRef: SubscriberRef = { - current: null, -}; +let subscriberRef: SubscriberRef = (null: any); + +if (enableSchedulerTracking) { + interactionsRef = { + current: new Set(), + }; + subscriberRef = { + current: null, + }; +} export {interactionsRef as __interactionsRef, subscriberRef as __subscriberRef}; diff --git a/packages/schedule/src/__tests__/Tracking-test.internal.js b/packages/schedule/src/__tests__/Tracking-test.internal.js index ce4b1973b4d..8de0cbc4858 100644 --- a/packages/schedule/src/__tests__/Tracking-test.internal.js +++ b/packages/schedule/src/__tests__/Tracking-test.internal.js @@ -365,5 +365,11 @@ describe('Tracking', () => { wrappedCallback(); }); + + describe('advanced integration', () => { + it('should not create unnecessary objects', () => { + expect(SchedulerTracking.__interactionsRef).toBe(null); + }); + }); }); }); diff --git a/packages/schedule/src/__tests__/Tracking-test.js b/packages/schedule/src/__tests__/Tracking-test.js index 2adecf1e3f7..6b340f8434e 100644 --- a/packages/schedule/src/__tests__/Tracking-test.js +++ b/packages/schedule/src/__tests__/Tracking-test.js @@ -48,15 +48,4 @@ describe('Tracking', () => { wrappedCallback(); }); - - // Make sure that accidental pairing of react-dom/profiling production bundle, - // With production (non-profiling) schedule/tracking doesn't cause runtime error. - it('should always export non-null refs to avoid breaking react-dom/profiling bundle', () => { - expect(SchedulerTracking.__interactionsRef).toEqual({ - current: new Set(), - }); - expect(SchedulerTracking.__subscriberRef).toEqual({ - current: null, - }); - }); }); From 8bfc0bf7a54afb5713317816b0b0f27366eef054 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 9 Sep 2018 09:37:34 -0700 Subject: [PATCH 4/6] Add invariant when profiling renderer is used with non-profiling schedule/tracking --- .../src/ReactFiberScheduler.js | 10 +++++++ .../__tests__/ReactTracking-test.internal.js | 28 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 20b2b4d4f85..e5162425892 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -168,6 +168,16 @@ let didWarnSetStateChildContext; let warnAboutUpdateOnUnmounted; let warnAboutInvalidUpdates; +if (enableSchedulerTracking) { + // Provide explicit error message when production+profiling bundle of e.g. react-dom + // Is used with production (non-profiling) bundle of schedule/tracking + invariant( + __interactionsRef != null && __interactionsRef.current != null, + "Profiling renderer (e.g. 'react-dom/profiling') cannot be used with non-profiling 'schedule/tracking'. " + + "To fix, alias 'schedule/tracking' to 'schedule/tracking-profiling'.", + ); +} + if (__DEV__) { didWarnAboutStateTransition = false; didWarnSetStateChildContext = false; diff --git a/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js b/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js new file mode 100644 index 00000000000..547d045c6a5 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js @@ -0,0 +1,28 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +'use strict'; + +describe('ReactTracking', () => { + it('should error if profiling renderer and non-profiling schedule/tracking bundles are combined', () => { + jest.resetModules(); + + const ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.enableSchedulerTracking = false; + + require('schedule/tracking'); + + ReactFeatureFlags.enableSchedulerTracking = true; + + expect(() => require('react-dom')).toThrow( + "Profiling renderer (e.g. 'react-dom/profiling') cannot be used with non-profiling 'schedule/tracking'.", + ); + }); +}); From f006b54adac1a2a8f92e0660eedffb0b8ba99976 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 10 Sep 2018 07:52:19 -0700 Subject: [PATCH 5/6] Improved tracking+renderer profiling bundle mismatch error message --- packages/react-reconciler/src/ReactFiberScheduler.js | 6 ++++-- .../src/__tests__/ReactTracking-test.internal.js | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index e5162425892..f1a13c34a3c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -173,8 +173,10 @@ if (enableSchedulerTracking) { // Is used with production (non-profiling) bundle of schedule/tracking invariant( __interactionsRef != null && __interactionsRef.current != null, - "Profiling renderer (e.g. 'react-dom/profiling') cannot be used with non-profiling 'schedule/tracking'. " + - "To fix, alias 'schedule/tracking' to 'schedule/tracking-profiling'.", + 'It is not supported to run the profiling version of a renderer (for example, `react-dom/profiling`) ' + + 'without also replacing the `schedule/tracking` module with `schedule/tracking-profiling`. ' + + 'Your bundler might have a setting for aliasing both modules. ' + + 'Learn more at http://fb.me/react-profiling', ); } diff --git a/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js b/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js index 547d045c6a5..3168f6f7785 100644 --- a/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactTracking-test.internal.js @@ -22,7 +22,7 @@ describe('ReactTracking', () => { ReactFeatureFlags.enableSchedulerTracking = true; expect(() => require('react-dom')).toThrow( - "Profiling renderer (e.g. 'react-dom/profiling') cannot be used with non-profiling 'schedule/tracking'.", + 'Learn more at http://fb.me/react-profiling', ); }); }); From 1168d347691ea0e4ef60edbd1cbc2dd9e2e39184 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 10 Sep 2018 08:31:51 -0700 Subject: [PATCH 6/6] Comment tweak --- packages/react-reconciler/src/ReactFiberScheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f1a13c34a3c..1a407aab556 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -170,7 +170,7 @@ let warnAboutInvalidUpdates; if (enableSchedulerTracking) { // Provide explicit error message when production+profiling bundle of e.g. react-dom - // Is used with production (non-profiling) bundle of schedule/tracking + // is used with production (non-profiling) bundle of schedule/tracking invariant( __interactionsRef != null && __interactionsRef.current != null, 'It is not supported to run the profiling version of a renderer (for example, `react-dom/profiling`) ' +