Aurelia Plugin doesn't use Webpack "entry" items as expected

[silbinarywolf]

I'm submitting a bug report

• Library Version: 3.0.0

Please tell us about your environment:

• Operating System: Windows 10

Current behavior: Aurelia currently only prepends special runtime files to the first entry defined in your Webpack config file.

'aurelia-webpack-plugin/runtime/empty-entry',
'aurelia-webpack-plugin/runtime/pal-loader-entry',

Snippet from node_modules\aurelia-webpack-plugin\dist\AureliaPlugin.js

addEntry(options, modules) {
    let webpackEntry = options.entry;
    let entries = Array.isArray(modules) ? modules : [modules];
    if (typeof webpackEntry == "object" && !Array.isArray(webpackEntry)) {
        // There are multiple entries defined in the config
        // Unless there was a particular configuration, we modify the first one
        // (note that JS enumerates props in the same order they were declared)
        // Modifying the first one only plays nice with the common pattern
        // `entry: { main, vendor }` some people use.
        let ks = this.options.entry || Object.keys(webpackEntry)[0];
        if (!Array.isArray(ks)) {
            ks = [ks];
        }
        ks.forEach(k => {
            webpackEntry[k] = entries.concat(webpackEntry[k])
        });
    } else {
        options.entry = entries.concat(webpackEntry);
    }
}

• What is the motivation / use case for changing the behavior?

Before Webpack 4, the fix/workaround above probably made sense. However, since vendoring is not handled in entry anymore, I think we can remove this without much fuss. I think ultimately it would help Aurelia be more predictable.

Making this change would require a major version bump as it breaks / changes behaviour.

addEntry(options, modules) {
    let webpackEntry = options.entry;
    let entries = Array.isArray(modules) ? modules : [modules];
    if (typeof webpackEntry == "object" && !Array.isArray(webpackEntry)) {
        for (let k in webpackEntry) {
            if (!webpackEntry.hasOwnProperty(k)) {
                continue;
            }
            let entry = webpackEntry[k];
            if (!Array.isArray(entry)) {
                entry = [entry];
            }
            webpackEntry[k] = entries.concat(entry)
        }
    } else {
        options.entry = entries.concat(webpackEntry);
    }
}

[jods4]

More background

To be fair, proper vendoring was never using entries (which are entry points, i.e. contain code that is meant to be executed when loaded).

Here's how you should have done vendoring in v3:

{
  plugins: [
    new webpack.optimize.CommonsChunkPlugin({
      name: ["runtime"],
      minChunks: Infinity,
    }),
    new webpack.optimize.CommonsChunkPlugin({
      name: "vendor",
      minChunks: ({ resource }) => /node_modules/.test(resource),
    }),
  ]
}

But reality is that many, many resources on the web abused entry: { main: "index", vendor: [...] } + CommonsChunkPlugin to achieve a similar result (which BTW would not work as well in more complex setups, e.g. when using multiple entry points).

This pattern is by far the most common one in Webpack v3-, which is why we have a hack to "make it work" by default in the plugin.

Webpack 4 now raises an error if you use CommonsChunkPlugin and forces users to change their pattern (remove vendor entries if they had one, use splitChunks instead).

My opinion

Since e49fb8dcbc86a90fb297d5b4a8974e14f41084a9 we don't support Webpack 3 anymore but require Webpack 4+.

As explained above, Webpack 4 forces you to stop using entry: { vendor }, so having a "fake" entry point should be really rare now, if ever.

In my opinion this means that we can safely remove the config hack. The upside is that multiple entry points will "just work" for people who do that, whereas today it needs an extra configuration.

Action

@EisenbergEffect I have one change I'd like to see in the PR, but once it's done if you agree with above can you merge and publish a release?

I am not sure that we need a major release. Yes it changes behavior in an observable way (all changes do) but I have a hard time coming up with a reasonable use case that would be broken.

[bigopon]

@silbinarywolf This is an interesting issue. May I know how you came to this?

[EisenbergEffect]

@jods4 Makes sense to me. Just ping me when it's all ready.

[silbinarywolf]

@jods4 My concern with making this a minor patch version is that it's very easy for someone to accidentally bump the version in the lock file which in-turn could cause their build to break horribly.

While I also can't imagine any possible breakage occurring due to this, I'd rather err on the side of caution so that a developer has to consciously know that something could break, to get this update.

I've lived through a lot of minor bumps that "weren't supposed to break anything" but did. Ultimately, it's up to you folks though!

@bigopon I came across this issue while trying to improve Cypress spec test bundling speed, as we experimentally use Cypress for unit testing Aurelia components. See: https://github.com/cypress-io/cypress/issues/3704#issuecomment-487782940 And our plugin: https://github.com/silbinarywolf/cypress-aurelia-unit-test