Improve DX when combining react-dom/profiling and schedule/tracking #13605

bvaughn · 2018-09-09T15:40:15Z

Resolves #13601

In order to be slightly smaller and faster, the schedule/tracking production bundle didn't initialize its refs. I hadn't properly considered the use case of react-dom/profiling importing a schedule/tracking (production, non-profiling) bundle– which runtime errors.

The "fix" for this includes:

996eb69: Add a new blessed entry point for schedule/tracking production+profiling bundle.
889a8de: Change schedule/tracking production bundle to also initialize the refs so react-dom production bundle wont' RTE. This should not impact react-dom production bundle because it doesn't even require schedule/tracking.
8bfc0bf: Add an invariant to protect against a react-dom profiling bundle being used with a non-profiling schedule/tracking

gaearon · 2018-09-09T15:46:28Z

Hmm. What is the recommendation for using profile mode? Is it to only use profiling bundle of ReactDOM? Or also to use profiling bundle of schedule?

If profiling needs profiling bundle for schedule, then it seems like we need a "blessed" entry point for it too. In other words we should have a profiling entry point for any package that needs it in profiling mode.

Otherwise it's not clear what makes react-dom special to deserve a nice entry point.

pull-bot · 2018-09-09T15:50:51Z

Details of bundled changes.

Comparing: 7d1169b...1168d34

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	+0.1%	+0.1%	645.88 KB	646.45 KB	151.37 KB	151.59 KB	UMD_DEV
react-dom.development.js	+0.1%	+0.2%	641.22 KB	641.82 KB	149.97 KB	150.2 KB	NODE_DEV
ReactDOM-dev.js	+0.1%	+0.1%	663.61 KB	664.25 KB	152.34 KB	152.55 KB	FB_WWW_DEV
react-dom.profiling.min.js	+0.1%	+0.1%	95.22 KB	95.29 KB	30.38 KB	30.39 KB	NODE_PROFILING
ReactDOM-profiling.js	0.0%	+0.1%	294.13 KB	294.25 KB	54.55 KB	54.57 KB	FB_WWW_PROFILING

react-art

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-art.development.js	+0.1%	+0.2%	439.51 KB	440.08 KB	98.53 KB	98.74 KB	UMD_DEV
react-art.development.js	+0.2%	+0.3%	371.26 KB	371.85 KB	81.41 KB	81.62 KB	NODE_DEV
ReactART-dev.js	+0.2%	+0.3%	376.26 KB	376.9 KB	80.17 KB	80.39 KB	FB_WWW_DEV

react-test-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-test-renderer.development.js	+0.1%	+0.3%	383.26 KB	383.83 KB	83.99 KB	84.21 KB	UMD_DEV
react-test-renderer.development.js	+0.2%	+0.3%	378.85 KB	379.44 KB	82.85 KB	83.07 KB	NODE_DEV
ReactTestRenderer-dev.js	+0.2%	+0.3%	383.97 KB	384.6 KB	81.88 KB	82.1 KB	FB_WWW_DEV

react-reconciler

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-reconciler.development.js	+0.2%	+0.3%	367.16 KB	367.75 KB	79.52 KB	79.74 KB	NODE_DEV
react-reconciler-persistent.development.js	+0.2%	+0.3%	365.7 KB	366.29 KB	78.95 KB	79.17 KB	NODE_DEV

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.2%	498.55 KB	499.17 KB	110.4 KB	110.61 KB	RN_FB_DEV
ReactNativeRenderer-dev.js	+0.1%	+0.2%	498.29 KB	498.91 KB	110.33 KB	110.54 KB	RN_OSS_DEV
ReactFabric-dev.js	+0.1%	+0.2%	488.74 KB	489.36 KB	107.98 KB	108.2 KB	RN_FB_DEV
ReactFabric-dev.js	+0.1%	+0.2%	488.77 KB	489.39 KB	108 KB	108.21 KB	RN_OSS_DEV
ReactNativeRenderer-profiling.js	+0.2%	+0.5%	212.25 KB	212.64 KB	37.19 KB	37.36 KB	RN_OSS_PROFILING
ReactFabric-profiling.js	+0.2%	+0.5%	203.89 KB	204.29 KB	35.69 KB	35.87 KB	RN_OSS_PROFILING
ReactNativeRenderer-profiling.js	+0.2%	+0.5%	220.73 KB	221.12 KB	38.68 KB	38.86 KB	RN_FB_PROFILING
ReactFabric-profiling.js	+0.2%	+0.5%	203.85 KB	204.25 KB	35.68 KB	35.86 KB	RN_FB_PROFILING

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

bvaughn · 2018-09-09T15:52:50Z

Yeah, I agree. First commit was just focused on fixing the runtime error. I'm adding another entry point now to the schedule package.

…filing bundle) to error

gaearon · 2018-09-09T16:04:00Z

I was thinking that maybe in this case the error is justified? While the error itself is confusing, you can google it and discover it comes from mixing profiling and non-profiling bundles. Whereas if we don't error, you would just be unaware they're inconsistent?

bvaughn · 2018-09-09T16:08:07Z

I've added a blessed entry-point for production+profiling schedule/tracking – although the name kind of sucks 😄

Verified it works by ejecting a CRA app and modifying the Webpack config to add the following aliases:

resolve: {
  alias: {
    'react-dom': 'react-dom/profiling',
    'schedule/tracking': 'schedule/tracking-profiling',
  },
}

bvaughn · 2018-09-09T16:09:14Z

I was thinking that maybe in this case the error is justified? While the error itself is confusing, you can google it and discover it comes from mixing profiling and non-profiling bundles. Whereas if we don't error, you would just be unaware they're inconsistent?

That's fair, unless we added a warning– you would only know something was wrong because of the fact that none of your interactions would be tracked.

bvaughn · 2018-09-09T16:12:50Z

I don't feel strongly about this. If you have a strong intuition that one route is better, I'll defer.

Reverting my initial commit would certainly be less subtle, and so maybe also less confusing.

gaearon · 2018-09-09T16:13:27Z

I think my preferred approach would be:

No code in production schedule that's unnecessary in production (like those Set allocations)
We have an explicit entry point for profiling version of schedule, the name you picked sounds fine
We add an invariant with a nice message to the profiling build of ReactDOM for cases if it's used with non-profiling schedule, and ask to alias them both

Seems like it would handle all cases and avoid extra code. Invalid mix would crash but it would explain how to fix it. Which you'd probably eventually want to do anyway since otherwise tracking wouldn't work.

bvaughn · 2018-09-09T16:16:48Z

Seems like it would handle all cases and avoid extra code

Except for the invariant itself, but if we're willing to add that– I agree that's probably the nicest experience.

…dom (profiling bundle) to error" This reverts commit 889a8de.

gaearon · 2018-09-09T16:25:55Z

Except for the invariant itself

That would be profiling-only, right? So wouldn't need to impact regular production bundle. I think that's acceptable.

bvaughn · 2018-09-09T16:28:14Z

True!

…dule/tracking

bvaughn · 2018-09-09T16:38:05Z

Okay. I've added an invariant and confirmed that it works with an ejected CRA. Would love input on the error message though. I'm not very good at writing these.

gaearon · 2018-09-09T18:16:21Z

packages/react-reconciler/src/ReactFiberScheduler.js

+  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'.",


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

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

The link points to a gist explaining how to do it in webpack. Later we change it to point to a blog post.

I dig it. 👍 Thanks for the ideas Dan.

gaearon · 2018-09-10T15:16:16Z

packages/react-reconciler/src/ReactFiberScheduler.js

@@ -168,6 +168,18 @@ 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


Nit: does this line need to start with capital letter? Made reading the sentence a bit confusing.

No, I can change the comment 😆

…acebook#13605) * Added blessed production+profiling entry point for schedule/tracking * Add invariant when profiling renderer is used with non-profiling schedule/tracking

bvaughn requested a review from gaearon September 9, 2018 15:40

facebook-github-bot added the CLA Signed label Sep 9, 2018

bvaughn force-pushed the react/issues/13601 branch from ddb265a to ecc6fea Compare September 9, 2018 15:42

schedule/tracking (production bundle) should not cause react-dom (pro…

889a8de

…filing bundle) to error

bvaughn force-pushed the react/issues/13601 branch from ecc6fea to 889a8de Compare September 9, 2018 15:57

Brian Vaughn added 2 commits September 9, 2018 09:18

Added blessed production+profiling entry point for schedule/tracking

996eb69

Revert "schedule/tracking (production bundle) should not cause react-…

16bef4c

…dom (profiling bundle) to error" This reverts commit 889a8de.

Add invariant when profiling renderer is used with non-profiling sche…

8bfc0bf

…dule/tracking

bvaughn changed the title ~~schedule/tracking (production bundle) should not cause react-dom (profiling bundle) to error~~ Improve DX when combining react-dom/profiling and schedule/tracking Sep 9, 2018

gaearon reviewed Sep 9, 2018

View reviewed changes

Improved tracking+renderer profiling bundle mismatch error message

f006b54

gaearon approved these changes Sep 10, 2018

View reviewed changes

Comment tweak

1168d34

bvaughn merged commit 03ab1ef into facebook:master Sep 10, 2018

bvaughn deleted the react/issues/13601 branch September 10, 2018 15:33

gaearon mentioned this pull request Sep 13, 2018

Add 16.5.1 changelog #13638

Merged

This was referenced Sep 14, 2018

Update react in / from 16.4.0 to 16.5.1 tjamet/photo-frontend#95

Open

Update react-dom in / from 16.2.0 to 16.5.1 LibreTubeApp/LibreTube#185

Open

dependencies bot mentioned this pull request Oct 1, 2018

Update react-dom in /modules/frontend from 16.3.0 to 16.5.2 project-accelerate/accelerate#277

Open

This was referenced Nov 1, 2018

Update react-dom in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#253

Closed

Update react in / from 16.2.0 to 16.6.0 leonardofaria/leozera-ui#254

Closed

This was referenced Dec 1, 2018

Update react-test-renderer in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#312

Open

Update react in /modules/frontend from 16.3.0 to 16.6.3 project-accelerate/accelerate#313

Open

heshamelmasry77 mentioned this pull request Jun 23, 2022

[Snyk] Security upgrade lighthouse from 2.0.0 to 9.3.0 heshamelmasry77/react#70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve DX when combining react-dom/profiling and schedule/tracking #13605

Improve DX when combining react-dom/profiling and schedule/tracking #13605

bvaughn commented Sep 9, 2018 •

edited

Loading

gaearon commented Sep 9, 2018 •

edited

Loading

pull-bot commented Sep 9, 2018 •

edited

Loading

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

schedule

bvaughn commented Sep 9, 2018

gaearon commented Sep 9, 2018

bvaughn commented Sep 9, 2018

bvaughn commented Sep 9, 2018

bvaughn commented Sep 9, 2018

gaearon commented Sep 9, 2018 •

edited

Loading

bvaughn commented Sep 9, 2018

gaearon commented Sep 9, 2018

bvaughn commented Sep 9, 2018

bvaughn commented Sep 9, 2018

gaearon Sep 9, 2018

bvaughn Sep 10, 2018

gaearon Sep 10, 2018

bvaughn Sep 10, 2018

Improve DX when combining react-dom/profiling and schedule/tracking #13605

Improve DX when combining react-dom/profiling and schedule/tracking #13605

Conversation

bvaughn commented Sep 9, 2018 • edited Loading

gaearon commented Sep 9, 2018 • edited Loading

pull-bot commented Sep 9, 2018 • edited Loading

react-dom

react-art

react-test-renderer

react-reconciler

react-native-renderer

schedule

bvaughn commented Sep 9, 2018

gaearon commented Sep 9, 2018

bvaughn commented Sep 9, 2018

bvaughn commented Sep 9, 2018

bvaughn commented Sep 9, 2018

gaearon commented Sep 9, 2018 • edited Loading

bvaughn commented Sep 9, 2018

gaearon commented Sep 9, 2018

bvaughn commented Sep 9, 2018

bvaughn commented Sep 9, 2018

gaearon Sep 9, 2018

Choose a reason for hiding this comment

bvaughn Sep 10, 2018

Choose a reason for hiding this comment

gaearon Sep 10, 2018

Choose a reason for hiding this comment

bvaughn Sep 10, 2018

Choose a reason for hiding this comment

bvaughn commented Sep 9, 2018 •

edited

Loading

gaearon commented Sep 9, 2018 •

edited

Loading

pull-bot commented Sep 9, 2018 •

edited

Loading

gaearon commented Sep 9, 2018 •

edited

Loading