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

html-template: Replace html-webpack-template with our own template #1049

Merged
merged 1 commit into from Aug 29, 2018
Merged

html-template: Replace html-webpack-template with our own template #1049

merged 1 commit into from Aug 29, 2018

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Aug 28, 2018

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 edmorley added this to the v9 milestone Aug 28, 2018
@edmorley edmorley self-assigned this Aug 28, 2018
@edmorley edmorley requested a review from eliperelman Aug 28, 2018
Copy link
Member

@eliperelman eliperelman left a comment

Did you say we can also still inject link and script tags via the config?

// @neutrinojs/html-template includes a custom template that has more features
// (eg appMountId and lang support) than the default html-webpack-plugin template:
// https://github.com/jantimon/html-webpack-plugin/blob/master/default_index.ejs
template: require.resolve('@neutrinojs/html-template/template.ejs'),
Copy link
Member

@eliperelman eliperelman Aug 28, 2018

Choose a reason for hiding this comment

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

Should we instead use path.resolve here?

Copy link
Member Author

@edmorley edmorley Aug 28, 2018

Choose a reason for hiding this comment

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

Yeah quite possibly

Copy link
Member Author

@edmorley edmorley Aug 29, 2018

Choose a reason for hiding this comment

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

Updated

@@ -1,5 +1,8 @@
import { resolve } from 'path';

Copy link
Member

@eliperelman eliperelman Aug 28, 2018

Choose a reason for hiding this comment

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

Let's remove the extra blank lines between imports.

Copy link
Member Author

@edmorley edmorley Aug 28, 2018

Choose a reason for hiding this comment

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

I intentionally added the newline, since I think it's cleaner to order (and separate) imports based on stdlib vs third-party vs local imports (we already do this for Treeherder). Ideally in the future all Neutrino imports would be sorted this way, and enforced via eslint-plugin-import.

@edmorley
Copy link
Member Author

@edmorley edmorley commented Aug 28, 2018

Did you say we can also still inject link and script tags via the config?

meta tags yes, via the meta option, link / script tags no (i'd mis-remembered) but there are plugins for at least script tags and people can use a custom template if needed (see reference in commit message about 80% case etc). Open to adding support for links iff an absolute blocker, but might be best to see what feedback we get without it first? (Easy to add retrospectively)

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 edmorley requested a review from eliperelman Aug 29, 2018
Copy link
Member

@eliperelman eliperelman left a comment

r+ with nits.

<!DOCTYPE html>
<html lang="<%= htmlWebpackPlugin.options.lang %>">
<head>
<meta charset="utf-8">
Copy link
Member

@eliperelman eliperelman Aug 29, 2018

Choose a reason for hiding this comment

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

It's super picky, but can you make this tag self-closing? The reasoning being xhtml mode. If they enable xhtml mode, this would be the one place in the template that isn't compliant, while a self-closing tag will be compliant with either mode.

Copy link
Member Author

@edmorley edmorley Aug 29, 2018

Choose a reason for hiding this comment

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

Copy link
Member

@eliperelman eliperelman Aug 29, 2018

Choose a reason for hiding this comment

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

Strange. It's funny that html-webpack-plugin would have an xhtml option for its users and not spit out fully-compliant markup for those users. Anyway, I'm willing to let this one go since its de facto on those platforms, so r+.

@edmorley edmorley merged commit f1da657 into neutrinojs:master Aug 29, 2018
2 checks passed
@edmorley edmorley deleted the replace-html-webpack-template branch Aug 29, 2018
edmorley added a commit that referenced this issue Oct 9, 2018
* An error is now shown when using the `links` HTML option without
  also specifying a custom template, so that users affected by #1049
  know what changes they need to make. See:
  #1129 (comment)
* The migration guide now recommends regenerating the lockfiles.
* The migration section about supported Node.js versions has been made
  clearer. See:
  #1129 (comment)
* The `neutrino` bin script error message now mentions `test` being
  removed, includes `--mode` for the inspect example, and uses the
  correct URL for the migration guide.
* The `minify.image` option deprecation error message no longer refers
  to `@neutrinojs/image-minify`, since it was removed. I've linked
  to the issue since it gives more background as to why compile-time
  minification is not recommended.
* The `hotEntries` option deprecation check now always occurs, and not
  only when `options.hot` is enabled. A test has also been added.
* Cleans up a few more `t.throws()` usages (missed in #1159).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants