Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Switch to Webpack 2 #737

Merged
merged 15 commits into from
Sep 13, 2016
Merged

Switch to Webpack 2 #737

merged 15 commits into from
Sep 13, 2016

Conversation

MoOx
Copy link
Owner

@MoOx MoOx commented Sep 8, 2016

We are blocked by mzgoddard/hard-source-webpack-plugin#27 (hmr + cache are not friends for now).

Otherwise all green!

MoOx added 10 commits September 8, 2016 13:45
You now must use webpack 2.
Don’t worry if it’s still tagged beta, it won’t it already usable for
months
and will be tagged as stable really soon.
This will mainly improve performance & size of the bundle
(which increased in recent version) thank to the
[tree shaking feature
](http://blog.sethladd.com/2013/01/minification-is-not-enough-you-need.h
tml).
You  might need to update some babel packages to latest version
(especially babel-preset-es2015, to get the `modules` option).

You will need to:

- add new sections in your ``babel.env`` configuration.
webpack 2 understand natively ``import``statements so it’s
unnecessary to
transform those (and it will allow webpack 2 enable tree shaking):
``webpack-develompment`` & ``webpack-production``.
Check out this commit for detail
(or take a look to the up to date boilerplate).
- disable ``webpack.optimize.DedupePlugin`` as it’s not working yet in
webpack 2
- update ``ExtractTextPlugin`` usage

Closes #421
@@ -1,26 +1,32 @@
// Hot loading HRM Patch
import "react-hot-loader/patch"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can UglifyJS remove this in production mode? Why don't you add this to devEntries in dev-server. We shouldn't require any changes in the boilerplate from user.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Currently, user need to explicitely use module.hot.accept (see below), so just moving this call is useless. Also if people have issue with RHL@3, they can replace it (or remove it). I will make some tests later to provides better default, but this will bring this kind of module as a dep (and we will need to use an option to enable/disable this kind of stuff).

And don't worry, the patch already have a dev/prod hook itself ;)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Previously, the call was still explicit in babel conf (in the webpack config)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Got it.

@thangngoc89
Copy link
Contributor

Hard source webpack plugin has a fix for us

@MoOx
Copy link
Owner Author

MoOx commented Sep 9, 2016

@thangngoc89 I just installed 0.0.35 but it does not fix things for us :(

@thangngoc89
Copy link
Contributor

Ouch.

@MoOx
Copy link
Owner Author

MoOx commented Sep 9, 2016

Wait a sec. It works with cache on (first pass, without no cache yet), but does not work for the 2nd build (when cache is used).

@thangngoc89
Copy link
Contributor

Could you report this back to HardSource?

@MoOx
Copy link
Owner Author

MoOx commented Sep 9, 2016

Done in the PR.

@MoOx
Copy link
Owner Author

MoOx commented Sep 9, 2016

Also it seems that webpack 2 does not magically avoid having unwanted stuff like everything related to the rendering engine (lowlight, highlight, lodash etc) which sucks. Will have to investigate.

@thangngoc89
Copy link
Contributor

thangngoc89 commented Sep 9, 2016

Also it seems that webpack 2 does not magically avoid having unwanted stuff like everything related to the rendering engine (lowlight, highlight, lodash etc) which sucks. Will have to investigate.

It's because you're using transpiled code when importing phenomic.

You could try adding js:next field to package.json like this https://github.com/reactjs/react-router/blob/28f693a1d7700a9efbba4f6c009ec5144d91fb5d/package.json#L13

@MoOx
Copy link
Owner Author

MoOx commented Sep 9, 2016

You are right. Then I will have to separate "phenomic" package before handling #598.

thangngoc89 added a commit to thangngoc89/dnh-cpp that referenced this pull request Sep 10, 2016
…phenomic"

To avoid shipping unnecessary code into the client bundle (regression
from 0.16.0), we removed plugins & presets from “phenomic” import.
Please directly use ``require(“phenomic/lib/loader-*``, by replacing
camelCase by dashedCase.

- ``phenomicLoaderPlugins.initBodyPropertyFromContent``
->
``require("./phenomic-loader-plugin-init-body-property-from-content").de
fault``,
- ``phenomicLoaderPlugins.initHeadPropertyFromConfig``
->
``require("./phenomic-loader-plugin-init-head-property-from-config").def
ault``,
- ``phenomicLoaderPlugins.initHeadPropertyFromContent``
->
``require("./phenomic-loader-plugin-init-head-property-from-content").de
fault``,
- ``phenomicLoaderPlugins.initRawPropertyFromContent``
->
``require("./phenomic-loader-plugin-init-raw-property-from-content").def
ault``,
- ``phenomicLoaderPlugins.initRawBodyPropertyFromContent``
->
``require("./phenomic-loader-plugin-init-rawBody-property-from-content")
.default``,
-
``phenomicLoaderPlugins.markdownInitHeadDescriptionPropertyFromContent``
->
``require("./phenomic-loader-plugin-markdown-init-head.description-prope
rty-from-content").default``,
- ``phenomicLoaderPlugins.markdownTransformBodyPropertyToHtml``
->
``require("./phenomic-loader-plugin-markdown-transform-body-property-to-
html").default``,
- ``phenomicLoaderPresets.default``
-> ``require("./phenomic-loader-preset-default").default``,
- ``phenomicLoaderPresets.markdown``
-> ``require("./phenomic-loader-preset-markdown").default``,
@MoOx
Copy link
Owner Author

MoOx commented Sep 10, 2016

  • "phenomic" import now do not have loader plugins & presets anymore
  • hmr seems to work

@MoOx
Copy link
Owner Author

MoOx commented Sep 10, 2016

Problem: Hot loading seems slower with webpack 2 + cache.
And also break our collection runtime "cache"... :'(

HMR + no cache

screen shot 2016-09-11 at 00 11 02

HMR + new cache

screen shot 2016-09-11 at 00 12 07

HMR + cache

1.1s the first time, but no collection is generated :@
This break our project since it seems that this file is cached https://github.com/MoOx/phenomic/blob/08bd04feb2254891caf809e383fca0f6608670c1/src/loader/cache.js and it should not because we use this as a "runtime" cache...

Poke @mzgoddard if you have an idea about this performance issue & this cache issue...

@MoOx MoOx added the blocked label Sep 10, 2016
@mzgoddard
Copy link

@MoOx I have a pretty good guess what is adding time after HardSource invalidates the cache. After the first build after that message from HardSource, if you restarted your server the build times will go down. Basically HardSource is serializing all of your modules every time after that first build, as opposed to just the ones that changed, because it doesn't know they're already serialized. I'm gonna make HardSource just a little smarter and that should keep it from re-serializing everything.

@MoOx
Copy link
Owner Author

MoOx commented Sep 11, 2016

@mzgoddard the thing is, during the start (with existing cache), a file is aggressively cached https://github.com/MoOx/phenomic/blob/08bd04feb2254891caf809e383fca0f6608670c1/src/loader/cache.js and our loader have no effect on it (it's currently adding all pages data into this object, but in practise, nothing is added, so the website have no page data :'(.

Any idea how we can fix this issue?

@MoOx
Copy link
Owner Author

MoOx commented Sep 11, 2016

Perf seems fixed with latest version of hard-source-webpack-plugin@0.0.37, but our runtime cache :/ Not sure how we can handle that... Or maybe it has something to do with require.context() caching... Will try to find where the issue comes from tonight.

@mzgoddard
Copy link

@MoOx I think you need to move the cache and feed creation into a plugin. You want part of how ExtractTextWebpackPlugin works. The stuff you are storing on cache should be stored on the module in the meta attribute that HardSource serializes. Then with that stored in meta, whether its the first run with HardSource or a different run from a preexisting cache your plugin can build your feeds during a compilation step like additional-assets. This should also remove what looks like race conditions to me with debouncing the feed work. (Is there an existing plugin that handles feed assets being in multiple built modules if the timeout debouncer runs with more upcoming modules that will run it a second time, or between multiple builds where a different module will have the feeds built the second time?)

So the first part of this is storing the info for your feeds like ExtractText:

var fs = require('fs');

// Use the path of the plugin/loader directory to avoid conflicts on the loaderContext.
var NS = fs.realpathSync(__dirname);

module.exports = function PhenomicLoaderPlugin(options) {
  this.options = options;
};

PhenomicLoaderPlugin.prototype.apply = function(compiler) {
  var options = this.options;

  compiler.plugin('compilation', function(compilation, params) {
    compilation.plugin('normal-module-loader', function(loaderContext, module) {
      loaderContext[NS] = function(loaderResult) {
        module.meta[NS] = loaderResult;
      };
    });

    compilation.plugin('additional-assets', function(callback) {
      // Collect results
      var results = compilation.modules.map(function(module) {
        return module.meta[NS];
      });

      // Build feeds
      Object.keys(options.feeds).forEach(function(name) {
        compilation.assets[name] = feed({
          // ...
        });
      });

      // Make sure to call callback or webpack will just stop because Node has
      // nothing left in the event queue to do.
      callback();
    });
  });
};

Then in place of storing on the cache you need to call that NS method in your loader.

var fs = require('fs');

// Use the path of the plugin/loader directory to avoid conflicts on the loaderContext.
var NS = fs.realpathSync(__dirname);

module.exports = function() {
  // Do needed loader work
  // ...

  // Make sure this data is ok to be JSON stringified and parsed.
  this[NS](result);

  // Any further loader work
  // ...
};

@MoOx
Copy link
Owner Author

MoOx commented Sep 11, 2016

The thing is: our loader is not just doing a feed, it's also generating json for each entry (md file) an return an object with the path to the emited file. And this cache is used for the static build, which avoid us to read data twice (but maybe we should read it twice...)
I will try to investigate but I guess we might end with a refactoring somewhere so your plugin work correctly with our project :(

@mzgoddard
Copy link

mzgoddard commented Sep 11, 2016

@MoOx keep using the loader to do those parts, to generate json per file and return the path to that file. Its just the feeds that need to be made by a plugin. This part of the loader needs to be done by a plugin. You don't need to read data twice, the plugin exposes a function your loader uses to save that data so the data can be reused by the plugin later. You could have a function on your plugin to expose the saved info to any other part of your build.

- Changed: RSS feeds should now be generated by
``PhenomicLoaderFeedWebpackPlugin``. You need to remove the ``feeds`` &
``feedsOptions`` sections from the loader configuration and instead add
them in a new webpack plugin ``new
PhenomicLoaderFeedWebpackPlugin({feedsOptions: …, feeds: { …``. You
will find the appropriate import in `import PhenomicLoaderWebpackPlugin
from “phenomic/lib/loader-feed-webpack-plugin”``.
@MoOx
Copy link
Owner Author

MoOx commented Sep 13, 2016

@mzgoddard your advices and webpack expertise were really precious. Now everything works as expected. Thank you so much for all your contributions.

@MoOx MoOx removed the blocked label Sep 13, 2016
@MoOx MoOx merged commit ea19f2c into master Sep 13, 2016
@MoOx MoOx deleted the webpack-2 branch September 13, 2016 22:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants