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

Support HMR #2

Merged
merged 3 commits into from Dec 16, 2016

Conversation

Projects
None yet
3 participants
@jasonLaster
Collaborator

jasonLaster commented Dec 1, 2016

This PR fixes a problem we see when Hot Module Reloading is running and some modules have different versions of react and therefore different injected header / footer stanzas.

One header version

/* WEBPACK VAR INJECTION */(function(module) {/* REACT HOT LOADER */ if (true) { (function () { var ReactHotAPI = __webpack_require__(190), RootInstanceProvider = __webpack_require__(198),        ReactMount = __webpack_require__(13), React = __webpack_require__(65); module.makeHot = module.hot.data ? module.hot.data.makeHot : ReactHotAPI(function () { return RootInstanceProvider.getRoot       Instances(ReactMount); }, React); })(); } try { (function () {

Another header version

/* WEBPACK VAR INJECTION */(function(module) {/* REACT HOT LOADER */ if (true) { (function () { var ReactHotAPI = __webpack_require__(3), RootInstanceProvider = __webpack_require__(11), Rea       ctMount = __webpack_require__(387), React = __webpack_require__(439); module.makeHot = module.hot.data ? module.hot.data.makeHot : ReactHotAPI(function () { return RootInstanceProvider.getRootI       nstances(ReactMount); }, React); })(); } try { (function () {

This fixes that problem by first sanitizing the modules.

Does this scale?

The sanitization is super verbose so that it is really fast. In our app webpack bootstrap went from 800ms to 1,300ms which is not that bad considering the extra work we do.

Add cache
caching lets us avoid two terrible things:
1. the loop over all modules
2. re-computing the sanitized body
@jasonLaster

This comment has been minimized.

Show comment
Hide comment
@jasonLaster

jasonLaster Dec 6, 2016

Collaborator

Just updated the PR to add a caching layer as well.

We needed this because firefox perf went to bootstrap times of 6 seconds. Caching brought it back down to 1 second.

Collaborator

jasonLaster commented Dec 6, 2016

Just updated the PR to add a caching layer as well.

We needed this because firefox perf went to bootstrap times of 6 seconds. Caching brought it back down to 1 second.

@bomsy

bomsy approved these changes Dec 7, 2016

@smikhalevski

@jasonLaster Thank you for this your fix! Can you please clarify what are those magic numbers?

@jasonLaster

This comment has been minimized.

Show comment
Hide comment
@jasonLaster

jasonLaster Dec 9, 2016

Collaborator

Sure, the magic numbers come from looking at the injected footer and loader and making some assumptions on their lengths.

For instance, a module does not have a header + footer and fewer than 400 chars...

Sorry for the complex code, honestly it works well for us on some big apps... but without tests its hard to promise it will work for everyone

Collaborator

jasonLaster commented Dec 9, 2016

Sure, the magic numbers come from looking at the injected footer and loader and making some assumptions on their lengths.

For instance, a module does not have a header + footer and fewer than 400 chars...

Sorry for the complex code, honestly it works well for us on some big apps... but without tests its hard to promise it will work for everyone

@jasonLaster

This comment has been minimized.

Show comment
Hide comment
@jasonLaster

jasonLaster Dec 9, 2016

Collaborator

@smikhalevski good news.

We were able to generalize the algorithm to avoid the hot reloading details as well as the magic numbers.

now we just strip away the requires and focus on the body text

Collaborator

jasonLaster commented Dec 9, 2016

@smikhalevski good news.

We were able to generalize the algorithm to avoid the hot reloading details as well as the magic numbers.

now we just strip away the requires and focus on the body text

@smikhalevski smikhalevski merged commit bf1dba0 into smikhalevski:master Dec 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment