-
Notifications
You must be signed in to change notification settings - Fork 47k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve DX when combining react-dom/profiling and schedule/tracking #13605
Changes from 4 commits
889a8de
996eb69
16bef4c
8bfc0bf
f006b54
1168d34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be bundler specific and I’m not sure they all use the “alias” wording. Another thing to consider is that the person seeing this might be unaware that schedule package exists or what it is. How about
The link points to a gist explaining how to do it in webpack. Later we change it to point to a blog post. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dig it. 👍 Thanks for the ideas Dan. |
||
); | ||
} | ||
|
||
if (__DEV__) { | ||
didWarnAboutStateTransition = false; | ||
didWarnSetStateChildContext = false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'.", | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ | |
"README.md", | ||
"index.js", | ||
"tracking.js", | ||
"tracking-profiling.js", | ||
"cjs/", | ||
"umd/" | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: does this line need to start with capital letter? Made reading the sentence a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I can change the comment 😆