Provide efficient webpack helpers for tree-shaking + DCE

[ryan-roemer]

Moving discussion from #547 to the broader one: how to provide end users with the most terse bundle. I forget how much we talked about this before, but we could do a package.json:browser or package.json:SOME_OTHER_MAIN that webpack picks up that points to src, but then run into the problem of everyone needing fully compatible babel builds.

1. Alias Option

One option is providing the resolves src paths as in the experiment repo's webpack config as a top-level provided thing so in your webpack config you could do something like:

const victorySrcAliases = require("victory/webpack").victorySrcAliases;
// {
//   "victory-core": "RESOLVE/PATH/victory-core/src",
//   "victory-chart": "RESOLVE/PATH/victory-chart/src",
//   // ...
// }

module.exports = {
  // ...
  module: {
    loaders: [
      {
        test: /\.js$/,
        include: [
          path.resolve("src"),
        ].concat(
          Object.keys(victorySrcAliases).map((k) => victorySrcAliases[k])
        )
        loader: "babel-loader"
      }
    ]
  },
  resolve: {
    alias: {
      // Force use of es6 src instead of es5 lib
      ...victorySrcAliases
    }
  },
  // ...
};

2. Different Main Option

A perhaps lighterweight custom idea is provide a custom victory specific namespace for: https://webpack.github.io/docs/configuration.html#resolve-packagemains that isn't in the defaults for webpack1: ["webpack", "browser", "web", "browserify", ["jam", "main"], "main"]

The reason why we need to avoid the defaults is otherwise everyone would need the victory-specific babel set up to be able to use victory, which isn't what we want.

So, like package.json:browser-victory that folks then explicitly add to webpack1 resolve.packageMains to then need + use babel building like in the experiment repo at: https://github.com/FormidableLabs/victory-line-experiments

// node_modules/victory*/package.json
"victory-browser": "src/index.js",
"main": "lib/index.js",

// my-project/webpack.config.js
resolve: {
  packageMains: ["victory-browser", "main"]
}

Also, we need a webpack2-amenable configuration that also works with the decided scheme: https://webpack.js.org/configuration/resolve/#resolve-mainfields

Current strawman:

"browser": "lib/index.js",
"main": "lib/index.js",
"module": "es/index.js",
"victory-module": "es/index.js"

• Requires transpiling to es directory (out of source, but published) like redux does.
• Consider a transform to convert es index imports to one-off-full-file-path imports -- https://bitbucket.org/amctheatres/babel-transform-imports -- to help mitigate Webpack tree shaking does not completely remove unused re-exports #549

For a good reference, see: https://github.com/reactjs/redux/blob/master/package.json

  "browser": "dist/redux.js",
  "main": "lib/index.js",
  "module": "es/index.js",
  "jsnext:main": "es/index.js",

  // ...
  "scripts": {
    "build:commonjs": "cross-env BABEL_ENV=commonjs babel src --out-dir lib",
    "build:es": "cross-env BABEL_ENV=es babel src --out-dir es",
    "build:umd": "cross-env BABEL_ENV=es NODE_ENV=development rollup -c -i src/index.js -o dist/redux.js",
    "build:umd:min": "cross-env BABEL_ENV=es NODE_ENV=production rollup -c -i src/index.js -o dist/redux.min.js",

Other Notes:

• Audit and confirm Victory unused stuff is dropped. We've seen stuff like the following from uglify that may indicate not dropped correctly? (Or maybe this warn is safe to ignore)

Side effects in initialization of unused variable __WEBPACK_IMPORTED_MODULE_0__victory_animation_victory_animation__ [main.js:94,25]

/cc @boygirl @chrisbolin @thomasthiebaud

[TomiS]

I'd like to drop in my two cents, as I'm currently trying to set up our app that uses Victory to use Webpack 2 with tree shaking... and I'm having big problems with it.

At the moment I'm having lots of errors like this when I'm trying to run a production build:

ERROR in ./~/d3-interpolate/src/basis.js
Module parse failed: /my_app/node_modules/babel-loader/lib/index.js??ref--0-0!/my_app/node_modules/d3-interpolate/src/basis.js Cannot read property 'name' of null
You may need an appropriate loader to handle this file type.
TypeError: Cannot read property 'name' of null
    at Parser.walkFunctionDeclaration (/my_app/node_modules/webpack/lib/Parser.js:539:39)
    at Parser.walkStatement (/my_app/node_modules/webpack/lib/Parser.js:436:32)
    at Parser.walkExportDefaultDeclaration (/my_app/node_modules/webpack/lib/Parser.js:613:9)
    at Parser.walkStatement (/my_app/node_modules/webpack/lib/Parser.js:436:32)
    at Parser.walkStatements (/my_app/node_modules/webpack/lib/Parser.js:419:9)
    at Parser.parse (/my_app/node_modules/webpack/lib/Parser.js:1193:8)
    at doBuild.e (/my_app/node_modules/webpack/lib/NormalModule.js:288:17)
    at runLoaders (/my_app/node_modules/webpack/lib/NormalModule.js:206:11)
    at /my_app/node_modules/loader-runner/lib/LoaderRunner.js:370:3
    at iterateNormalLoaders (/my_app/node_modules/loader-runner/lib/LoaderRunner.js:211:10)
    at iterateNormalLoaders (/my_app/node_modules/loader-runner/lib/LoaderRunner.js:218:10)
    at /my_app/node_modules/loader-runner/lib/LoaderRunner.js:233:3
    at context.callback (/my_app/node_modules/loader-runner/lib/LoaderRunner.js:111:13)
    at /my_app/node_modules/babel-loader/lib/index.js:159:14
    at /my_app/node_modules/babel-loader/lib/fs-cache.js:76:24
    at /my_app/node_modules/babel-loader/lib/fs-cache.js:31:14
    at Gunzip.onEnd (zlib.js:227:5)
    at emitNone (events.js:91:20)
    at Gunzip.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)
 @ ./~/d3-interpolate/index.js 3:0-58
 @ ./~/victory-core/lib/victory-animation/util.js
 @ ./~/victory-core/lib/victory-animation/victory-animation.js
 @ ./~/victory-core/lib/index.js
 @ ./~/victory/src/index.js
 @ ./src/charts/styles/MyChart.js

Previously I thought I have a problem with D3 but it seems the ES2015 (Harmony) module support of D3 is already implemented. However, this thread makes me think that the reason might be Victory.

I've understood that to make tree shaking work in Webpack 2, one needs to set

// webpack.config.js
{
    resolve: {
        mainFields: ['module', 'browser', 'main']
    }
}

This configures Webpack to primarily use ES2015 modules provided by 3rd party packages instead of other builds.

So... to answer the question of this issue, I think Victory should also provide module as an main field option in package.json and of course a proper build matching that.

ps. I'm willing to help (at least by testing) because this issue is quite urgent for us.

[ryan-roemer]

@TomiS -- A near-term hack for this that should work is available at: https://github.com/FormidableLabs/victory-line-experiments/blob/master/webpack.config.js

(You'll need to (1) implement a resolveSrc around all victory* modules you are using in webpack config, (2) place them all as top-level dependency in your package.json (otherwise you'll be relying on unpredictable npm flattening which could break without warning, and (3) add the paths to both the babel-loader and resolve.alias in webpack config. Obviously this is a lot of hacking you don't want long term, but it does work now.)

And I think we can turn this around the general solution fairly quickly if we get a little bandwidth...

[ryan-roemer]

Also, to properly set expectations with webpack2 tree shaking, there are still underlying issues with webpack itself to getting to a completely DCE'd final bundle, discussed here: #549

(But we should at least get rid of the "everything problem" with using the es5 lib that is currently the default for all builds)

[TomiS]

@ryan-roemer Cool, thanks. I managed to get it working. In addition to what you said, for my build I also had to 1) include all D3 modules to babel-loader, 2) add babel include and alias also for victory main library. Victory main library needs to be the last alias of them, I think 3) Use stage-2 preset in babel 4) Add babel plugin transform-export-default-name (This is really weird, btw, but I don't seem to get D3 imports work otherwise)

And after all this, it seems the bundle is larger than with regular build :D It didn't surprise though, as I've read about similar experiences in other threads.

[ryan-roemer]

Yeah, while initially such a super hyped-up thing, tree-shaking really is a fickle beast. I think in parallel to all of this finding a solution to #549 will likely have us end up with an internal refactor along the lines of @thomasthiebaud 's suggestions in #547 where we: admit tooling defeat and just do all the one-off, full-path imports for everything but the deliberate multiple re-export index.js files.

[TomiS]

@ryan-roemer Not sure if this helps, but were you aware of this plugin: https://www.npmjs.com/package/babel-plugin-transform-imports

[ryan-roemer]

@TomiS -- Not specifically, so thanks for the reference!

I've been on private projects where someone wrote a more-tailored version of exactly this that relied on guaranteed conventions in file naming / export patterns. I was offhand even thinking we could do a bespoke version for Victory. I will definitely consider this as an easier alternative to full internal one-off refactor!

[TomiS]

@ryan-roemer Nice to hear it might be helpful. I just stumbled upon that today and installed it. At least it seems to work for react-bootstrap and lodash that were used as an example in the docs. Haven't tried adding other libs yet though.