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

ng build production should minify index.html in dist #11360

Closed
CharlBest opened this issue Jun 25, 2018 · 26 comments
Closed

ng build production should minify index.html in dist #11360

CharlBest opened this issue Jun 25, 2018 · 26 comments
Labels
area: devkit/build-angular devkit/build-angular:browser feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Milestone

Comments

@CharlBest
Copy link

CharlBest commented Jun 25, 2018

Bug Report or Feature Request (mark with an x)

- [] bug report -> please search issues before submitting
- [x] feature request

Area

- [x] devkit
- [ ] schematics

Versions

node -- version = v8.10.0
node -- version = v5.6.0

Repro steps

ng new proj
cd proj
ng build --prod

The log given by the failure

index.html in dist is not minified

Desired functionality

Minify the index.html in production dist

Mention any other details that might be useful

  • I'm sure it did this in previous versions of the CLI
  • I ran my site through https://developers.google.com/speed/pagespeed/insights and it suggested to minify my HTML. So that's how I ended up here.
    41593038-9e25ca9e-73b6-11e8-861f-a7f8bcff4e0d
  • I'm not sure if this is related to this previous commit (feat(@angular/cli): add support for minifying HTML): 75311c2

PS: it's so cool how so many of these best practices and enhancement the CLI does out of the box for you. Thanks for that!

@CharlBest CharlBest changed the title ng build production not minifying index.html any more ng build production should minify index.html in dist Jun 27, 2018
@CharlBest
Copy link
Author

Hi @Brocco. I would just like to know how issues like these gets considered to be done?

@filipesilva filipesilva added the feature Issue that requests a new feature label Aug 10, 2018
@robindijkhof
Copy link
Contributor

I thought Angular4 was able to do this?

@CharlBest
Copy link
Author

AFAIK it was but was disabled as they made big changes in the core code (I think) and didn't have time to put it back in. But 8 votes so far is looking good so hopefully it will be considered soon.

@gilhanan
Copy link

I'm not sure if it will help someone, but worth try.
I found a workaround by use the html-minifier npm package and run it after the build have finished:
npm install --save-dev html-minifier

package.json:

"postbuild": "node index-minifier.js"

index-minifier.js:

var fs = require('fs');
var minify = require('html-minifier').minify;

var filePath = 'dist/index.html';

fs.readFile(filePath, function (err, buf) {
  if (err) {
    console.log(err);
  } else {

    var originalValue = buf.toString();
    var minifiedValue = minify(originalValue, {
      caseSensitive: false,
      collapseBooleanAttributes: true,
      collapseInlineTagWhitespace: false,
      collapseWhitespace: true,
      conservativeCollapse: false,
      decodeEntities: true,
      html5: true,
      includeAutoGeneratedTags: false,
      keepClosingSlash: false,
      minifyCSS: true,
      minifyJS: true,
      preserveLineBreaks: false,
      preventAttributesEscaping: false,
      processConditionalComments: true,
      processScripts: ["text/html"],
      removeAttributeQuotes: true,
      removeComments: true,
      removeEmptyAttributes: true,
      removeEmptyElements: false,
      removeOptionalTags: true,
      removeRedundantAttributes: true,
      removeScriptTypeAttributes: true,
      removeStyleLinkTypeAttributes: true,
      removeTagWhitespace: true,
      sortAttributes: true,
      sortClassName: true,
      trimCustomFragments: true,
      useShortDoctype: true
    });


    fs.writeFile(filePath, minifiedValue, function (err, data) {
      if (err) {
        console.log(err)
      } else {
        var diff = originalValue.length - minifiedValue.length;
        var savings = originalValue.length ? (100 * diff / originalValue.length).toFixed(2) : 0;
        console.log("Successfully minified '" + filePath + "'" +
          ". Original size: " + originalValue.length +
          ". Minified size: " + minifiedValue.length +
          ". Savings: " + diff + " (" + savings + "%)");
      }
    });
  }
});

@ngbot ngbot bot added this to the Backlog milestone Jan 24, 2019
@JounQin
Copy link
Contributor

JounQin commented Sep 6, 2019

{
  "postbuild": "html-minifier dist/static/index.html --collapse-whitespace --remove-comments --remove-optional-tags --remove-redundant-attributes --remove-script-type-attributes --remove-tag-whitespace --use-short-doctype -o dist/static/index.html"
}

It should be simplest solution to me.

@MilesBHuff
Copy link

This should be built-in to Angular itself; we shouldn't have to configure a separate library to do this.

@e-oz
Copy link

e-oz commented Feb 13, 2020

It's definitely not a bug. I don't see why this feature should be implemented at all.

@rafa-suagu
Copy link

It's definitely not a bug. I don't see why this feature should be implemented at all.

It's simple, it's the same reason to apply the minification to CSS and JS.

This bug/feature must be present in combination with this issue: #8759

@e-oz
Copy link

e-oz commented Feb 13, 2020

It's pretty special file, so if such minification could produce any issues/bugs - better to avoid it.
I don't think many single-page angular apps have a lot of content in index.html so I don't see actual benefits - few bytes of saved space?

@rafa-suagu
Copy link

I'm sorry but I don't understand how the premise that a feature can cause bugs leads you to think that it can be ignored. In addition, your statement is contradicted because if index.html is such a controlled environment, it should not cause problems to treat it.

Finally, Google (through speed insights) warns you to minify the html with the consequent positioning problems. Angular is not just a framework to use in admin apps. Some of us need to take in count the Google tips to have good positioning.

@e-oz
Copy link

e-oz commented Feb 13, 2020

I'm sorry but I don't understand how the premise that a feature can cause bugs leads you to think that it can be ignored

It is always the case. If feature gives less benefits than issues, then it should not be implemented.

Angular is not just a framework to use in admin apps. Some of us

And who of us was talking about admin apps? Could you quote please?

Finally, Google (through speed insights) warns you to minify the html

When using Angular, almost all of your HTML content is not in index.html.

@rafa-suagu
Copy link

@e-oz I just said the "Angular is not just a framework to use in admin apps" because the unique case to ignore this Google tip is when you are developing an admin app.

One thing I learned by developing public apps (not admins), is that if you want to be a good site for Google, you must follow the Google tips. If Google says: Minify your html, you must minify it. You can't answer: I'm sorry Google, "almost of our html content is not in index.html"

Anyway, I think a discussion about it in 2020 makes no sense.

If you want to know if Angular Team wants or not this feature you can see the comments here: 75311c2#commitcomment-29616137

I have nothing more to say about it, just wait to Angular Team implements it to be able to use it

@e-oz
Copy link

e-oz commented Feb 13, 2020

Lol, this comment actually reveals my suspicions. Thank you.

If Google says: Minify your html, you must minify it.

And it's exactly what is happening.

You can't answer: I'm sorry Google, "almost of our html content is not in index.html"

You absolutely can, don't lie.

@HoseinGhanbari
Copy link

HoseinGhanbari commented Apr 12, 2020

for those who load some javascripts (e.g. gtag.js)

npm i html-minifier --save-dev

node_modules/.bin/html-minifier dist/[app-name]/index.html --collapse-whitespace --remove-comments --remove-optional-tags --remove-redundant-attributes --remove-script-type-attributes --remove-tag-whitespace --use-short-doctype --minify-css true --minify-js true -o dist/[app-name]/index.html

@dgp1130
Copy link
Collaborator

dgp1130 commented May 15, 2020

Discussed this in our CLI triage meeting. We recognize that optimizing index.html is a clear area of improvement for Angular, and understand the desire for automated metrics to accurately reflect the state of the application. We previously did support minification of index.html but were forced to disable it due to tooling incompatibilities at the time. It also slowed down builds, required additional dependencies with varying levels of support, and generally increased complexity of the CLI.

To implement this properly, we need to minify the file as a post-process step in order to support differential loading and localization. The CLI doesn't have a great way of doing that right now, so we need to update our internal infra to better support this. There is the possibility of leveraging the Angular compiler for HTML minification (rather than an external dep), since it already needs to parse HTML for Angular templates, but that will require coordination with the compiler to support. Whether using ngc to minify HTML is actually good idea is still TBD.

In summary, minifying index.html probably isn't that hard by itself, but to do it correctly would require a non-trivial amount of infrastructure work to support.

I ran PageSpeed Insights on angular.io, and I believe the HTML minification check has been removed (JS and CSS checks are present, but no HTML check was passed or failed). If someone is still seeing automated systems complaining about this, let me know. A potential 40% page size decrease sounds useful, but when it's only a few hundred bytes (before compression), the tool should be surfacing other, more impactful optimizations.

Screenshot from 2020-05-15 10-36-51

It seems that an HTML minification audit was considered by PageSpeed Insights but dropped as it was not considered impactful enough to justify the cost. Considering PageSpeed's mission of making a faster web, the amount of research and investigation they put into these questions, and the fact that this wasn't significant enough to implement, minifying an already trivial index.html file won't have a noticeable impact to users.

Considering the high infrastructure cost and the low impact of the feature I don't expect this to be prioritized soon. There are other benefits to adding the discussed CLI infrastructure, and when that gets prioritized and implemented we can revisit index.html minification as the implementation and maintenance cost will become much lower.

@dgp1130 dgp1130 removed the needs: discussion On the agenda for team meeting to determine next steps label May 15, 2020
@gkalpak
Copy link
Member

gkalpak commented May 17, 2020

A note in case this gets implemented (some day 😛):
For apps using @angular/service-worker, we should ensure that the SW manifest is built after the minification, so that the content-based hashes are correct in the manifest's hash table. See #17700 (and references in there) for details.

@Tanja-4732
Copy link

This issue has been assigned to the backlog milestone on Jan 24, 2019.

Has there been any progress since then?

@MilesBHuff
Copy link

@Bernd-L #11360 (comment)

@mac2000
Copy link
Contributor

mac2000 commented Jul 20, 2020

In case anybody is still looking for this kind of functionality there is another approach, you gonna need @angular-builders/custom-webpack and provide your indexTransfrom e.g.:

angular.json

{
  // ...
  "architect": {
    "build": {
      // "builder": "@angular-devkit/build-angular:browser",
      "builder": "@angular-builders/custom-webpack:browser",
      "options": {
        "indexTransform": "./path/to/index.transform.ts",
        // ...
      }
    }

  }
}

index.transform.ts

import { TargetOptions } from '@angular-builders/custom-webpack'
import { minify } from 'html-minifier'

export default (targetOptions: TargetOptions, indexHtml: string) => minify(indexHtml, {
    removeComments: true,
    collapseWhitespace: true,
    minifyCSS: 'clean-css'
  })

Benefit of this is that you do not need any additional npm scripts and as a result wont forget to run them

@vmasek
Copy link

vmasek commented Mar 2, 2021

@mac2000 thanks for your post, it inspired us to do the same thing & we improved on it a bit 🙂

  1. The main improvement is in using actively maintained fork of HTML minifier https://github.com/terser/html-minifier-terser
  2. We also wanted to skip the minification on local serve as it brings no benefit and adds minification time to each hot reload serve

index.transform.ts

import { TargetOptions } from '@angular-builders/custom-webpack';
import { minify } from 'html-minifier-terser';

export default (options: TargetOptions, indexHtml: string) =>
  options.target === 'serve'
    ? indexHtml
    : minify(indexHtml, {
        caseSensitive: true,
        collapseWhitespace: true,
        minifyCSS: true,
        minifyJS: true,
        removeComments: true,
        removeRedundantAttributes: true,
        removeScriptTypeAttributes: true,
        useShortDoctype: true,
      });

@naveedahmed1
Copy link

BTW is there any specific reason that team (apparently, since the issue was opened in 2018) wants to keep the index.html un-minified?

@MilesBHuff
Copy link

MilesBHuff commented Mar 10, 2021

@naveedahmed1 #11360 (comment)
It's not that they want to; it's just it's a lot of work to do it right, and the benefits for doing so are marginal. They're hoping to get to it eventually; but in the mean-time, they're working on other, more-impactful optimizations.

Check out this comment for a reasonable way to implement it just in your project(s): #11360 (comment)

@hiepxanh
Copy link
Contributor

I compared the hash of the file index.html on server, it was different from the value in 'ngsw.json' file.

    sha1sum index.html
    04e691dbfae931bfd24513ac63d833bdfee3a1f6  index.html

After I disabled the line endings transformation:

git config --local core.autocrlf false

and push project again problem disappeared and sha1sum index.html showed me the save hash as in the 'ngsw.json'.

@alan-agius4
Copy link
Collaborator

From #22559 by @jelbourn

While working on angular.io, I noticed that the site includes a number of both HTML and inline JS comments. That led me to note that the HTML and inline JS includes whitespace. In the grand scheme of things this is pretty minor, but it would be nice if the CLI could apply a few basic, safe optimizations for index.html:

  • Remove comments (probably configurable for HTML comments which can theoretically be load-bearing)
  • Remove non-significant whitespace (e.g. any whitespace outside of <body>)
  • Optimize inline JS (needs some due diligence to make sure this is safe) and CSS (much safer)

@angular-robot angular-robot bot added the feature: under consideration Feature request for which voting has completed and the request is now under consideration label Feb 1, 2022
@ngbot ngbot bot modified the milestones: Backlog, needsTriage Feb 1, 2022
@dgp1130
Copy link
Collaborator

dgp1130 commented Feb 17, 2022

Discussed this some more today, though I don't think our opinion has changed much since #11360 (comment).

Generally speaking, we don't think the improvements here are worth the cost of a new dependency. Angular apps have very small index pages (since it's all client-side rendered), so the benefits are minimal. HTML compresses really well and is very fast to parse in browsers. Lighthouse came to the same conclusion, which I think backs up the argument that this is very low impact.

To address the specific points @jelbourn mentioned:

  • Remove comments (probably configurable for HTML comments which can theoretically be load-bearing)

We could possibly do this without a complex dependency (html.replace(/<!--.*-->/, '')), I'm not sure if that's comprehensive enough for this case. Even if we did, comments could be load bearing in a very awkward way. The most notable are legacy "if IE" comments, which still exist. Angular doesn't support IE, but an "if IE" comment is a pretty reasonable way of communicating to users that your app doesn't support IE and they need to upgrade, so breaking that would be quite annoying. We could add options for some of this, or to turn off the feature altogether, but that adds additional complexity which we'd rather not take on for such a feature.

If we did strip comments, we would likely be stuck with that decision for the foreseeable future, since developers could put confidential information in those comments with the expectation that gets removed before being shipped to users. Reintroducing such comments could cause a lot of problems for users, meaning we would likely be stuck with stripping comments forever, even if we decided the cost/benefit of the feature wasn't actually worth it.

  • Remove non-significant whitespace (e.g. any whitespace outside of <body>)

This almost certainly requires a dependency that actually parses HTML. Whitespace can be significant in some locations and not in others. HTML5 also has a lot of non-obvious quirks (ie. certain, arbitrary tags are self-closing). Again, the impact is not worth the increased complexity.

  • Optimize inline JS (needs some due diligence to make sure this is safe) and CSS (much safer)

Inline JS and CSS outside of Angular are generally not recommended. They require CSP exceptions and aren't optimized as mentioned. If we did support this, it would require additional options to configure what optimizations are performed. It also couples it to the optimizers we currently use for standard app builds, and would make changing those optimizers harder if we ever choose to do so in the future.

Our recommendation is to avoid inline JS / CSS as much as possible and implement those use cases in the Angular application itself. This may not always be possible, and if there are specific use cases which require it, I recommend you file a separate FR for that where we can better support you. For the edge cases that do rely on inline scripts, they should be small enough that such optimizations would have minimal impact anyways.

Angular Universal is one case where optimizing HTML has more value since it actually contains the page contents rather than a single tag like typical Angular builds use. However the rendered HTML generated by Angular already has its contents minified, so a separate optimization pass for the whole page has just as minimal impact.

@dgp1130 dgp1130 closed this as completed Feb 17, 2022
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: devkit/build-angular devkit/build-angular:browser feature: under consideration Feature request for which voting has completed and the request is now under consideration feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests