Skip to content
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

Merged
merged 6 commits into from Sep 10, 2018

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 9, 2018

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
@bvaughn bvaughn requested a review from gaearon Sep 9, 2018
@bvaughn bvaughn force-pushed the react/issues/13601 branch from ddb265a to ecc6fea Sep 9, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 9, 2018

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.

Loading

@pull-bot
Copy link

@pull-bot pull-bot commented Sep 9, 2018

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

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

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

Loading

@bvaughn bvaughn force-pushed the react/issues/13601 branch from ecc6fea to 889a8de Sep 9, 2018
@gaearon
Copy link
Member

@gaearon gaearon commented Sep 9, 2018

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?

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

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',
  },
}

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

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.

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

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.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Sep 9, 2018

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.

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

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.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented Sep 9, 2018

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.

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

True!

Loading

@bvaughn
Copy link
Contributor Author

@bvaughn bvaughn commented Sep 9, 2018

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.

Loading

@bvaughn 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
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'.",
Copy link
Member

@gaearon gaearon Sep 9, 2018

Choose a reason for hiding this comment

The 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

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.

Loading

Copy link
Contributor Author

@bvaughn bvaughn Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig it. 👍 Thanks for the ideas Dan.

Loading

@@ -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
Copy link
Member

@gaearon gaearon Sep 10, 2018

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.

Loading

Copy link
Contributor Author

@bvaughn bvaughn Sep 10, 2018

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 😆

Loading

@bvaughn bvaughn merged commit 03ab1ef into facebook:master Sep 10, 2018
1 check was pending
Loading
@bvaughn bvaughn deleted the react/issues/13601 branch Sep 10, 2018
@gaearon gaearon mentioned this pull request Sep 13, 2018
Simek added a commit to Simek/react that referenced this issue Oct 25, 2018
…acebook#13605)

* Added blessed production+profiling entry point for schedule/tracking
* Add invariant when profiling renderer is used with non-profiling schedule/tracking
jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
…acebook#13605)

* Added blessed production+profiling entry point for schedule/tracking
* Add invariant when profiling renderer is used with non-profiling schedule/tracking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants