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

Switch to using 'inject: true' and not duplicating upstream functionality #61

Closed
edmorley opened this issue Oct 17, 2017 · 8 comments · May be fixed by #83
Closed

Switch to using 'inject: true' and not duplicating upstream functionality #61

edmorley opened this issue Oct 17, 2017 · 8 comments · May be fixed by #83

Comments

@edmorley
Copy link

edmorley commented Oct 17, 2017

The upstream html-webpack-plugin plugin used to include most logic in the actual template file, however in v2.29.0 switched to injecting the relevant markup into the template using JS (see jantimon/html-webpack-plugin@c8a6925).

Currently html-webpack-template must be used with that JS injection mode turned off (inject: false), since it instead re-implements much of the upstream functionality in the custom index.ejs.

This results in a few issues:

  • it breaks the plugin/event functionality of html-webpack-plugin (as noted in FYI: Not compatible with HtmlWebpackInlineChunkPlugin #48), which will become increasingly important as more plugins become available.
  • it means the upstream functionality has to be re-implemented/duplicated here, and this package doesn't automatically benefit from any bug fixes/new features from upstream (which are likely more frequent given the popularity of the upstream package).
  • it means there's a chance for slight behaviour differences for the features that exist both in upstream and this template, which could cause confusion when people switch between the two.

I would propose:

  • updating the docs here to say to use inject: true (this will require a major version bump)
  • removing the parts of index.ejs that are handled upstream, leaving just the custom funcationality

As an added bonus these changes will make it clearer what functionality this custom template adds, and then if in the future any of those features are upstreamed, this package can be simplified even further :-)

Thoughts? (If this sounds like something that would be accepted, I'm happy to open a PR.)

@jaketrent
Copy link
Owner

@edmorley thanks for outlining this. The situation and proposed direction are clear and helpful to me.

I love the idea of slimming this template and no duplicating upstream functionality. I know little about the fundamentals of what is broken and how changing inject true is going to fix it. Perhaps you could point me toward some basic education on the subject.

I'm in favor of moving in this direction and would welcome a PR.

@jaketrent
Copy link
Owner

Hey @edmorley I still like the idea of this -- not duplicating upstream functionality. I would welcome your expertise in adjusting it. Are you still interested? If not, no problem at all, and we can close this. What do you think?

@edmorley
Copy link
Author

Hi! Sorry for the delayed reply. I'll try and take a look at a proof of concept PR this week :-)

@merlinnot
Copy link

Hi @edmorley , did you manage to take a look on this?

@alex-shamshurin
Copy link

Without inject = true this plugin break other plugins. I played some hours with it and realize that I can use it.

edmorley added a commit to neutrinojs/neutrino that referenced this issue Aug 29, 2018
…1049)

The HTML template that comes with `html-webpack-plugin` [1] is very
minimal and so until now we've used the more fully-featured template
from `html-webpack-template` [2] instead.

However that template currently reimplements the core functionality
of `html-webpack-plugin` in template markup and so has to be used
with `inject` mode disabled [3]. This means:
- many `html-webpack-plugin` plugins don't work (eg the one for SRI)
- the template depends much more strongly on the internal data
  structures of `html-webpack-plugin`, and so can be broken by new
  major versions (as will be the case with `html-webpack-plugin` v4)
- the template doesn't benefit from upstream improvements made to
  the injected content.

In addition, `html-webpack-plugin` now supports several features that
were previously only possible with a custom template (eg additional
meta tags), and there are now many third party plugins to provide
additional functionality (and are the preferred approach).

As such, this switches `@neutrinojs/html-template` to using its own
custom template, that is virtually the same as [1], other than adding
`lang` and `appMountId` support. The `mobile` option has been replaced
by adding the same viewport tag via the new `meta` option.

If there are features that `html-webpack-template` supported, that
our new simpler template does not, users can use their own template
(`html-webpack-plugin` pushes this approach strongly) - Neutrino
should cater for the 80% out of the box, not the 99%).

Unrelated to the switch away from `html-webpack-template`, I've also
stopped setting `xhtml` to `true`, since XHTML compliant output isn't
something that most people need.

[1] https://github.com/jantimon/html-webpack-plugin/blob/master/default_index.ejs
[2] https://github.com/jaketrent/html-webpack-template/blob/master/index.ejs
[3] jaketrent/html-webpack-template#61
@edmorley
Copy link
Author

Sorry for not having time to look at this. I believe it's something that's definitely worth doing, since in addition to the issues described in the OP, it caused breakage with this package when trying it with the new html-webpack-plugin v4 alpha. Those issues have since been fixed (html-webpack-plugin rolled back the breaking changes for now), however it proved how fragile inject: false is.

The Neutrino project has since switched away from using this package, so I probably won't have a chance to look at this - however happy to look over a PR if someone else does.

A first step to fixing this issue would be if someone could take a look at adding tests to this repository, that verified the output HTML for various combinations of options. Then when fixing this issue, it would be much easier to (a) review, (b) ensure no unexpected breakage.

@alex-shamshurin
Copy link

Right now I'm developing the same with handlebars engine and it will be possible to use it with "inject = true" and it much cleaner solution. All these "<% %>" impossible to read.

@edmorley
Copy link
Author

A first step to fixing this issue would be if someone could take a look at adding tests to this repository,

I've filed #79 for this

vladimyr added a commit to ExtensionEngine/tailor that referenced this issue Oct 8, 2019
- removing `html-webpack-template` until
  jaketrent/html-webpack-template#61
  gets resolved
@edmorley edmorley closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants