Reduce stats requested at emit and optionally remove from template

[niftymonkey]

At Trello we have been dealing with some build speed issues related to building multiple locale specific HTML files. These issues are similar to what's described here.

After instrumenting this plugin, I found that the majority of the time spent here is in the two toJson calls that are done. This change modifies the stats we request in the initial toJson call in the emit callback and also provides a constructor property that allows plugin users to opt out of the toJson call that is done in the template handler. In our case, we didn't need access to that webpack variable in our template, and therefore did not need the overhead of the toJson call.

[gerges-zz]

For the Trello team, these changes reduced the HML generation step of our build from 20+ seconds to 3. We'd love to see these changes merged upstream.

[gregjacobs]

Hey @niftymonkey, instead of a constructor property to prevent the .toJson() call for the template, could you instead make the webpack property into a getter that lazily calls stats.toJson()?

Then the performance hit is only for people using the webpack property, without the extra plugin option to worry about

[gerges-zz]

@gregjacobs the issue with attempting to make this property lazy has to do with how the internal template loader works. Take a look at this line: https://github.com/jantimon/html-webpack-plugin/blob/master/lib/loader.js#L46

That line of code writes out function scoped vars to make accessing template params not require another call. Part of the code it generates will include var webpack = templateParams.webpack;, which is going to call a get accessor defined on templateParams, 'webpack'. You could rewrite that code to in turn define all those properties as lazy as well on global, but things start to get ugly.

Would love some direction from maintainers on the route they'd like to go. Is this actively maintained @jantimon / @mastilver ?

[gregjacobs]

@gerges Oh ugh.. Didn't realize it was implemented that way.

One way to get around that might be to just surround the template code in a with statement:

'module.exports = function (templateParams) {' +
    'with(templateParams) {' +
        // Execute the lodash template
        'return (' + template.source + ')();' +
    '}' +
'}';

Although the with keyword is no longer allowed in strict mode, so that would only run if the rest of the code wasn't running under strict mode.

Might be best to have a plugin option after all

EDIT: Never mind on the with statement. Apparently that was the first attempt in the codebase anyway, but there is a comment in that source file that says that Webpack 2 doesn't allow the with statement, and hence the workaround of writing out the variables individually

[gregjacobs]

Or another option would be an AST transform of the template source to prepend templateParams. to each identifier which corresponds to a key in the templateParams object.

Maybe more effort than it's worth..

[niftymonkey]

@gregjacobs Yeah, that's kinda where we landed. Anything past this did seem a little more effort than it was worth to to solve the problem initially. Again though, as @gerges mentioned, if there's anything the maintainers need to have done before we can get this merged, I'd be happy to oblige.

cc: @jantimon @ampedandwired

[gregjacobs]

@jantimon, @mastilver please merge this PR

[niftymonkey]

Any chance for a merge on this one @jantimon ?

[jantimon]

What about the following solution:

#830 (comment)

It would allow us to remove it from the template but also allow users who really need it to add it back in

[jantimon]

l’ll close this issue for now - please let me know if the proposed solution might not work.

[jantimon]

Implemented in 3.1.0.

Please try templateParameters: false to overwrite the template variables entirely

[n0v1]

I think this could be simplified according to the docs:

var chunkOnlyConfig = {
  all: false,
  chunks: true
};

[jantimon]

Cool good point! 👍
We will remove the getStats call in #953

[n0v1]

Even better 👍. Thanks for your work!

[lock]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.