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

Feature/loaders #41

Merged
merged 6 commits into from
Jan 20, 2016
Merged

Feature/loaders #41

merged 6 commits into from
Jan 20, 2016

Conversation

jantimon
Copy link
Owner

This rather big pull request will allow to use loaders #10 :

plugins: [
    new HtmlWebpackPlugin({
      template: 'html!./index.html'
    })
  ]

Further changes to get this working:

  • It will remove the deprecated templateParams.htmlWebpackPlugin.assets.
  • It will also remove the templateContent feature. (Maybe we can fix this)
  • It adds an examples - for easier issue debugging and for people to try the plugin
  • It sets inject to true by default

This merge request is work in progress and will need some more testing and cleanup.

@SimenB
Copy link
Contributor

SimenB commented May 18, 2015

Oh, this is awesome!

Regarding compile, how about flipping it and calling it preCompiled (precompiled?)?

@jantimon
Copy link
Owner Author

I thought something like postprocess / postProcess. It could become a little bit the replacement for the missing templateContent option.

@jantimon
Copy link
Owner Author

Open todos:

  • Move blueimp into a loader (blueimp!template.html) - replaces compile option
  • More tests
  • Bring back templateContent

options: self.options,
}
};
if (self.options.compile === true) {
Copy link

Choose a reason for hiding this comment

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

Why not to allow compiler to be replaced instead of using a flag? That seems like a more flexible alternative to me. It could still default to blue imp for backwards compatibility.

If you want to skip compiling altogether, you would just set compiler: id (pass through).

Copy link
Owner Author

Choose a reason for hiding this comment

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

hey @bebraw thanks for the feedback..
Actually I have a new (hopefully) better idea.
Instead of using a template processing part I will use a blueimp template loader by default.
This will reduce the complexity and will still support blueimp templates

Copy link

Choose a reason for hiding this comment

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

Cool. I'm a happy camper if I can write something like require('html!highlight!markdown!./README.md') within the template and have it evaluated through Webpack using those loaders.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This should already work with this branch however it's not finalised

@jantimon
Copy link
Owner Author

jantimon commented Jun 3, 2015

@bebraw I have just added an example which uses the blueimp loader together with the html loader.
https://github.com/ampedandwired/html-webpack-plugin/tree/feature/loaders/examples/two-loaders

This is just an example but should give you an idea how loaders might work together.

@SimenB
Copy link
Contributor

SimenB commented Jun 3, 2015

This looks really promising, can't wait for it to be released! Anything I/we can do to speed this up?

A thought, why not drop the blueimp dep, and just go with lodash templates?

@jantimon
Copy link
Owner Author

jantimon commented Jun 3, 2015

@ampedandwired chose blueimp - and I would rather concentrate on the loaders issue first.
Furthermore you could just use lodash templates with the lodash loader ;)

@SimenB
Copy link
Contributor

SimenB commented Jun 3, 2015

I use handlebars or jade, so it makes no real difference to me. Just thinking that more people are probably familiar with lodash/underscore templates than blueimp. It also allows you to drop a, albeit small, dependency.

But yeah, different issue 😄

@jantimon
Copy link
Owner Author

jantimon commented Jun 3, 2015

Before releasing I would like to add more examples and use those examples for integration testing.

So it would be a great support if you could add some more examples (e.g. underscore, jade, handlebars, mocha-loader, using the chunks option and so on)

@bebraw
Copy link

bebraw commented Jun 3, 2015

@jantimon That {% include(require('html!./partial.html'), {}); %} looks nice! Great work. 👍

@saberduck
Copy link

I have troubles using jade as template, because I have jade registered in loaders array in webpack.config.js and when I use it for the template it is loaded with blueimp-tmpl. I believe this is because no loaders are detected in the template name? I think, perhaps blueimp-tmpl shouldn't be used by default, only when requested explicitly. Or at least provide option to disable it

@jantimon
Copy link
Owner Author

@saberduck This is for backwards compatibility.. Maybe we could add the blue-imp loader to the module loaders so it would become a fallback - but I am not sure if this would would work.

@saberduck
Copy link

Maybe, if you can replace this test

     if(this.options.template.indexOf('!') === -1) {
       this.options.template = 'blueimp-tmpl!' + path.resolve(this.options.template);
     }

with some explicit option like use-blueimp , which is true by default, but configurable

I think I can workaround it by using raw! , but this would be nicer imho

@jantimon
Copy link
Owner Author

Why can't you just use jade!template.html ?

Right now the 1.x branch uses blueimp templates by default.
For this reason I chose also to use the blueimp templating by default for this branch.
Otherwise it would brake every current implementation.

@saberduck
Copy link

because I have jade loader defined as loader for .jade files in module.loaders array. If I specify jade! with template, it will try to process file with jade loader twice and it doesn't work. However jade!raw! works, but seems like unnecessary hack.

@jantimon
Copy link
Owner Author

thanks for the reporting - I have to look into that 👍
I don't understand why it would load jade twice...

@saberduck
Copy link

I will try to setup some minimalistic project to show you my problem, maybe I am missing something and problem is somewhere else, ttyl

@jantimon
Copy link
Owner Author

Cool - let me know I'll look into it

@jantimon
Copy link
Owner Author

@saberduck - fixed that. It will use only a loader if you haven't specified one

@kbanman
Copy link

kbanman commented Jul 4, 2015

Excellent work! I'm very much looking forward to being able to use this

@saberduck
Copy link

Looks great now, thanks! Sorry, I didn't have time for the minimal test-case to reproduce the issue I had before

@rayshan
Copy link

rayshan commented Dec 17, 2015

@SimenB do you mean like below, with a custom key name?

new HtmlWebpackPlugin({
    template: "app/index.html",
    xxx: "favicon.png"
})

<link rel="shortcut icon" href=/<%= htmlWebpackPlugin.options.xxx %>>

The result is what I would expect:
<link rel="shortcut icon" href="/favicon.png">

But previously with the favicon key, the favicon file gets copied over. With a custom key, it doesn't.

@SimenB
Copy link
Contributor

SimenB commented Dec 17, 2015

Just

new HtmlWebpackPlugin({
    template: "app/index.html"
})

and the link tag as

<link rel="shortcut icon" href="./path/to/favicon.ico">

That works with hbs at least, should be the same for lodash

@rayshan
Copy link

rayshan commented Dec 17, 2015

@SimenB thanks for getting back. With your code, it's just hard coding to a predetermined path, no? I suppose we would use it with the file-loader to move the favicon.

@SimenB
Copy link
Contributor

SimenB commented Dec 18, 2015

Yup, that's right. I prefer having it in code and not in configuration, anyways 😄
You could always use a resolve.alias if you prefer that. In my exp, the favicon option is unecesarry if you provide your own template in version 2

@jeshtan
Copy link

jeshtan commented Dec 26, 2015

Does template's loader support query syntax? For example

This work

new HtmlWebpackPlugin({
    template: "first?hello=world!./app/index.html"
})

But

new HtmlWebpackPlugin({
    template: "second!first?hello=world!./app/index.html"
})

Don't work

@mxstbr
Copy link

mxstbr commented Jan 12, 2016

@jantimon 2.6.0, getting the following error when I require(html-webpack-plugin). Maybe related to @rayshan's comment above?

Error: Cannot find module './lib/compiler.js'
    at Function.Module._resolveFilename (module.js:337:15)
    at Function.Module._load (module.js:287:25)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at Object.<anonymous> (/Users/a2517/sites/personal/github/react-boilerplate/node_modules/html-webpack-plugin/index.js:7:21)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)
    at Module.require (module.js:366:17)
    at require (module.js:385:17)
    at Object.<anonymous> (/Users/a2517/sites/personal/github/react-boilerplate/webpack/makewebpackconfig.js:6:25)
    at Module._compile (module.js:425:26)
    at Object.Module._extensions..js (module.js:432:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:311:12)

The important part:

html-webpack-plugin/index.js:7:21

@jantimon
Copy link
Owner Author

@mxstbr thanks for your feedback #164 was just fixed with c6cb00c

@SimenB
Copy link
Contributor

SimenB commented Jan 12, 2016

@jantimon You can include the whole lib-directory to avoid the problem in the future 😄

@jantimon
Copy link
Owner Author

@SimenB Good point 👍 will do :)

@SimenB
Copy link
Contributor

SimenB commented Jan 12, 2016

Created a PR for it

@haf
Copy link

haf commented Jan 14, 2016

I've been trying to grok the samples and this PR's code, but haven't made it yet. I want to transform this: <link rel="apple-touch-icon" href="/img/favicon.png" /> into <link rel="apple-touch-icon" href="/2de345f67ff65a43.png" /> where the alpha-numeric string is the webpack-compiled name. Since the html page is not using JS, I can't just do require('./img/favicon.png'), so how would I do it? An ico is not enough nowadays.

@SpaceK33z
Copy link
Contributor

@haf, you can use something like html-loader or underscore-template-loader for this. When you have this installed, you can just use a <link href="x.png"> and it will automatically compile it and change the url to the correct path.

@jantimon
Copy link
Owner Author

@SpaceK33z considering backwards compatibility and stability of underscore-template-loader - is there any reason why we should not use underscore-template-loader as default loader for this plugin?

@SpaceK33z
Copy link
Contributor

@jantimon, I don't think there's a good reason not to use it. It's lightweight and I think stable enough. Using it as default loader would certainly be interesting!

@jantimon
Copy link
Owner Author

I am about to release 2.x - do you see any open points which should be addressed first?

@mxstbr
Copy link

mxstbr commented Jan 20, 2016

@jantimon do it!

@SpaceK33z
Copy link
Contributor

Yay! Please do :)

@jantimon jantimon merged commit a35e5e3 into master Jan 20, 2016
@mxstbr
Copy link

mxstbr commented Jan 20, 2016

Fuck yeah, good work @jantimon! 🎉🎉🎉

@SimenB
Copy link
Contributor

SimenB commented Jan 21, 2016

We had an issue in development on rebuilds after jumping from 2.0.4 to 2.6.something (don't remember exact version ottomh) when using custom handlebars.
I'm on the bus on my way to work now, so I'll see if it's still a problem with the latest version

@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.

None yet