Open vladimyr opened 5 years ago
Hey @vladimyr , how are you feeling about these changes that you've made (for the better of the project)? And about the requested adjustment for your meta implementation?
I'd love to get this merged in for all to enjoy.
Hi @jaketrent 👋
Sorry for being unresponsive. I had to shift my priorities but rest assured that I'm also eager to get it merged so community gets benefit from it but me too because I'm also using it inside my own projects.
There is one breaking change that I've noticed that I'd like to revert on: that's the removal of arbitrary meta tags. I'd like to get that back.
Um, I did that because html-webpack-plugin
already supports meta injection (see meta
option: https://github.com/jantimon/html-webpack-plugin#options) and my concern wasn't really duplicating functionality instead I'm not sure we can keep it around without conflicting with html-webpack-plugin
's option. html-webpack-template
's meta
is an array while html-webpack-plugin
expects dictionary object. I haven't tested it but my guess is that html-webpack-plugin
won't pass meta
down to the html-webpack-template
. Instead it will fail due to wrong data format being provided? 🤔 Long story short, I need to do double check that but I think we are forced to make this change.
Are there other breaking changes that I'm not noticing? If so, I'd love to give those parts another review too.
I removed baseHref
following the same logic - html-wepack-plugin
already supports that through base
option (see base
option: https://github.com/jantimon/html-webpack-plugin#options). There aren't data format changes so it is merely matter of rename from baseHref
to base
.
Same applies to favicon
that can be specified using html-webpack-plugin
's favicon
option (see favicon
option: https://github.com/jantimon/html-webpack-plugin#options). It used to be a string, html-webpack-plugin
expects a string, so no changes there.
I altered <link>
tag generation to make it controllable by xhtml
html-webpack-plugin
's option (see xhtml
option: https://github.com/jantimon/html-webpack-plugin#options). If xhtml
compatibility mode is explicitly requested links will end up self closed, otherwise they'll be generated as usual. Previously links were always XHTML compatible.
I removed devServer
option because injecting dev server client is webpack-dev-server
's duty and is something that works out of box without requiring any template changes.
I removed inlineManifestWebpackName
option because there is plugin listed inside html-webpack-plugin
's docs that does exactly that so we don't need to keep that logic around.
I believe those are all breaking changes.
The addition of the test suite is great! Is there a test that exercises the main premise of this PR, being able to inject via html-webpack-plugin?
Well that is basically every test that has been added? Each test uses webpack programmatically and compiles spec/fixtures/index.js
using dynamically generated webpack config that includes both html-webpack-plugin
& html-webpack-template
settings. Results (js bundle and generated html) are being written to in-memory filesystem and tested using regex patterns to verify that template settings are being correctly applied. They are basically e2e tests matching real life workflow and making sure html-webpack-template
fits well there.
Hey @vladimyr , how are you feeling about these changes that you've made (for the better of the project)?
Great? 😁 Only reason I haven't turned this from draft to proper PR yet are missing documentation changes. Docs really need some love and having updating guide would be really nice. I have two ideas regarding docs:
v6
and new v7
? docs same way how webpack-chain
does that: https://github.com/neutrinojs/webpack-chain#readme but I'm not sure how to do that as part of (single) PR. It requires creating new v6
branch (from current master
) that readme could point to.html-webpack-plugin
's layout for describing options (https://github.com/jantimon/html-webpack-plugin#options) to reduce reader's cognitive load. I'm guessing that people will read those simultaneously so keeping it similar may help readers. What do you think?Long story short I would personally like to see necessary documentation changes happen too. I'm willing to make those but can't guarantee on ETA. If you want you can merge it in current state so we make it 2-step upgrade or you can wait for me finishing it first which is totally up to you! 🎉
@vladimyr Thanks for the additional context on the changes you made. Please don't feel pressure on an ETA. This is all for the love of the project, right. Please take care of your other life priorities first.
Regarding the breaking changes, here's what I'm understanding: You removed meta
, baseHref
, and favicon
because html-webpack-plugin
already handles this directly. Great!
link
change makes sense.
devServer
and inlineManifestWebpackName
removal because of handling elsewhere also makes sense.
With the additional info, I'm all for these changes.
I think what you point out is correct, we need a bit of documentation to point people in the right direction for these options if they were previously expecting to find them here.
And regarding your documentation points:
html-webpack-plugin
. I'm all for using that format.Good luck as you get back around to this. If it sits for a long while, that's also just fine. Thanks again.
🚧 This resolves #61 & #79