Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for [hash] in filename #257

Closed
wants to merge 2 commits into from
Closed

Conversation

dotch
Copy link

@dotch dotch commented Mar 16, 2016

This PR adds support for replacing [hash] in the filename with the hash of the compiled template.

This behavior was brought up in #121 and would be pretty helpful for me.

@@ -37,7 +37,7 @@ HtmlWebpackPlugin.prototype.apply = function (compiler) {

compiler.plugin('make', function (compilation, callback) {
// Compile the template (queued)
compilationPromise = childCompiler.compileTemplate(self.options.template, compiler.context, self.options.filename, compilation)
compilationPromise = childCompiler.compileTemplate(self.options.template, compiler.context, 'template', compilation)
Copy link
Author

Choose a reason for hiding this comment

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

The filename was replaced here, because the child compiler would otherwise replace the [hash] by itself, causing an error. The actual provided filename for this function seems to not matter and be a temporary thing anyway. Please correct me if my assumption is wrong.

Copy link
Owner

Choose a reason for hiding this comment

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

Webpack will overwrite an existing file with the name 'template'. This could also conflict if you would have two instances of the webpack-html-plugin.

Copy link
Author

Choose a reason for hiding this comment

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

Hm I guess that makes my approach rather pointless. We can not hand in the filename with the hash before we even have compiled the template.

@jantimon
Copy link
Owner

Is this different from #249?

@dotch
Copy link
Author

dotch commented Mar 17, 2016

Yes, this PR targets the output html file, #249 targets the included assets.
An example usage for this PR would be:

new HtmlWebpackPlugin({
      template: 'index.html.template',
      filename: 'build/index-[hash].html',
}),

[hash] would then be replaced with the hash of the compiled template.
This is useful in cases where the final output html is by itself only an asset that also needs cache busting. E.g. when using it as an iFrame or with html-imports.

@@ -48,6 +48,7 @@ HtmlWebpackPlugin.prototype.apply = function (compiler) {
// If the compilation change didnt change the cache is valid
isCompilationCached = compilationResult.hash && self.childCompilerHash === compilationResult.hash;
self.childCompilerHash = compilationResult.hash;
self.options.filename = self.options.filename.replace(/\[hash\]/g, compilationResult.hash);
Copy link
Owner

Choose a reason for hiding this comment

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

Do we have to change the user options here?

Copy link
Author

Choose a reason for hiding this comment

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

We could store the filename in a different property such as this.modifiedFilename and leave the options untouched for clarity.

Copy link
Owner

Choose a reason for hiding this comment

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

Can't we just use self.childCompilerHash ?

@dotch
Copy link
Author

dotch commented Mar 18, 2016

I added a commit using self.childCompilerHash to build the filename at the position where it gets used.

@jantimon
Copy link
Owner

Looks quite good - however I am still not sure about the hardcoded template name.
It might cause issues with recompilation and caching.
Could we instead just escape the template name?

Maybe you could also add a test to the caching spec.

@dotch
Copy link
Author

dotch commented Mar 24, 2016

Agree with both of your points. Will address them and document the functionality in the readme.

@dotch dotch force-pushed the master branch 2 times, most recently from f56103a to 3287c5a Compare April 1, 2016 01:39
@dotch
Copy link
Author

dotch commented Apr 1, 2016

@jantimon Mind having another look at this?
I 'escape' the filename before handing it to the child compiler, by just removing [hash] from the String.
I added a test for correct handling of [hash] in the output file name.
And I run the whole caching spec suite with a [hash] in the filename.
Sorry for the massive diff due to the indentation change in CachingSpec.js.

@krukid
Copy link

krukid commented Apr 1, 2016

@dotch This PR is exactly what I need for an IFrame, but one thing is not clear to me: when the plugin generates a hash-based file name, how do I get it programmatically? I mean ideally, I could just require(file_name) (or template_name?) and get the compiled HTML asset URL, but something tells me that is not how it works. Am I wrong?

@jantimon
Copy link
Owner

jantimon commented Apr 1, 2016

Could you please revert the indentation change - it's really impossible to read otherwise.

@dotch
Copy link
Author

dotch commented Apr 1, 2016

@jantimon
Sorry about that. I reverted the whitespace changes and put them in a second commit for now.

@jantimon
Copy link
Owner

jantimon commented Apr 2, 2016

Looks like it is still broken

@@ -43,8 +43,12 @@ HtmlWebpackPlugin.prototype.apply = function (compiler) {
}

compiler.plugin('make', function (compilation, callback) {
// Remove occurences of '[hash]' in the filename so the child
// compiler does not try to replace it.
var escapedFilename = self.options.filename.replace('[hash]', '');
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldnt it be better to replace [ and ] to reduce the chance of collisions and support also other webpack variables?

We could move the escaping to a small file in the library folder which does only the webpack filename escaping and unescaping.

What about [ -> {[{ and ] -> }]} ?

Should still be a valid filename: https://en.wikipedia.org/wiki/Filename

@jantimon
Copy link
Owner

jantimon commented Apr 2, 2016

After some research I tried to use the webpack way of handling the filename: #279.
What do you think?

@dotch
Copy link
Author

dotch commented Apr 6, 2016

Looks good to me, I like that approach!

@jantimon
Copy link
Owner

jantimon commented Apr 7, 2016

Moved to #279

@jantimon jantimon closed this Apr 7, 2016
@lock
Copy link

lock bot commented May 31, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2018
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.

4 participants