webpack updates #1120
webpack updates #1120
Conversation
salvatore-fxpig
force-pushed
the
webpack-updates-3
branch
from
b3398ed
to
afdf3f9
Compare
|
The check fail triggered my curiosity a bit about the packaging. There's a couple of "easy" optimizations in there that could save a couple of k ( If you have strong concerns for bundle size, have you considered keeping a separate vendor bundle ? It would not affect worst-case loading time at all, but it would improve average loading time significantly. |
|
As alternative to loading modules: ["main" ...] one could babelize node_modules, lots of people doing that now, takes a bit more to compile but should be safer from the point of view of adding new extensions in the future, and saves a bit of package size as well. |
salvatore-fxpig
force-pushed
the
webpack-updates-3
branch
from
afdf3f9
to
613bc5c
Compare
There was a problem hiding this comment.
Thanks for this -- there are a lot of webpack improvements possible within auspice & it's been a while since I dived into this.
There's a couple of "easy" optimizations in there that could save a couple of k (react-select, d3-range), then a couple more complex ones (chunking away react-ga and the locales folder.
Would like to explore these, but caching is probably a better investment right now. I've just seen #1126 which does all of this!
If you have strong concerns for bundle size, have you considered keeping a separate vendor bundle ? It would not affect worst-case loading time at all, but it would improve average loading time significantly.
I think we need to be doing this in the medium-term future, but it'll add in an extra layer of complexity which is why I haven't explored it yet. Adding hashes to the bundles and allowing them to be cached may be a better solution, or one which should be implemented first (currently the bundles have a cache I've just seen #1126 which does all of this!max-age=0)
[regression] the hot-reloading implementation in this PR exhibits strange behavior in that the first modification to source code triggers a bundle rebuild & the browser automatically updates-in-place, however subsequent modifications to source code don't update the browser-in-place (refreshing the browser must be done). Seems to be a regression compared with current master.
[bug] this doesn't build when auspice is used as a dependency of a project, with auspice-client-customisations which import react via:
import React from "react"; ERROR in /Users/naboo/github/nextstrain/nextstrain.org/auspice-client/customisations/navbar.js
Module not found: Error: Can't resolve 'react' in '/Users/naboo/github/nextstrain/nextstrain.org/auspice-client/customisations'
@ /Users/naboo/github/nextstrain/nextstrain.org/auspice-client/customisations/navbar.js 20:0-26 89:22-27 97:22-27 102:24-29 112:22-27 147:26-31 217:2-7 228:22-27 230:18-23 234:18-23 238:74-79 242:19-24 244:42-47 248:18-23 252:103-108 254:181-186
@ /Users/naboo/github/nextstrain/nextstrain.org/auspice-client/customisations sync ^\.\/.*$
@ ./src/util/extensions.js
@ ./src/util/googleAnalytics.js
@ ./src/index.js
@ multi babel-polyfill ./src/index
To reproduce, you can clone the nextstrain.org repo using branch testing-auspice-pr-1120 and run the following:
npm install
npm run build
webpack.config.js
Outdated
| "@libraries": path.join(__dirname, 'node_modules') | ||
| "@libraries": path.join(__dirname, 'node_modules'), | ||
| // Pins all react stuff to auspice dir, and uses hot loader's dom (can be used safely in production) | ||
| "react": path.join(__dirname, 'node_modules/react'), // eslint-disable-line quote-props |
There was a problem hiding this comment.
When auspice is a dependency of a project X, then __dirname does point to the correct location (X/node_modules/auspice) however react is not present within X/node_modules/auspice/node_modules for reasons unknown to me.
I think this approach is the right one in order to ensure that customisations use the same libraries as "core" auspice. My original idea was to force customisations to import code via import react from "@libraries/react" but this also had problems. I remember that approach worked when auspice was a global install, but not when it was a "normal" dependency, and these were perhaps the same issues as we're seeing now.
There was a problem hiding this comment.
I've had a look and while it feels like a bit of a "nuclear" option, explicitly looking for the "root" node_modules folder in the webpack config seems to do the trick.
As for the hot loader thing, could you verify if #1126 has the same problem ? I remember resolving this but it was fairly entangled with the react-i18n module IIRC so if it works in #1126 I'd rather not do it twice.
There was a problem hiding this comment.
Seems by some lucky combination the hot loader actually works well without using the hot-loader dom nor the patch/import, so I pushed a micro commit that reverts those changes and removes the regression.
A more extensive, hopefully more stable solution using the "recommended" implementation of react-hot-loader is found in #1126 and requires a bit of fiddling with react-i18n.
There was a problem hiding this comment.
#1126 still isn't perfect, but i'd rather pursue the stable solution, even if we do it in a subsequent PR.
|
Going to close this, as #1126 is much more complete (albeit with a few tweaks needed). |
Description of proposed changes
This pull request changes some elements in webpack config, namely:
reactand react utilities to allow better compatibility with out-of-root-folder extensionsreact-hot-loadereval-source-mapbecause why not go with the best quality if it's only for debugging (note: I tried debugging performance usingcheap-module-source-mapand it was utterly impossible, everything was mapped to the bundles xD)Related issue(s)
#902
Testing
A general smoke test should suffice probably... !
Thank you for contributing to Nextstrain!