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

Add support for minifying index.html #5753

Merged
merged 1 commit into from
Jun 7, 2017
Merged

Add support for minifying index.html #5753

merged 1 commit into from
Jun 7, 2017

Conversation

ishitatsuyuki
Copy link
Contributor

@ishitatsuyuki ishitatsuyuki commented Mar 30, 2017

This was built with compatibility in mind.

@ishitatsuyuki ishitatsuyuki changed the title [wip] add support for minifying HTML Add support for minifying HTML Mar 30, 2017
@ishitatsuyuki
Copy link
Contributor Author

Travis spurious error. Please trigger a rebuild.

@dave11mj
Copy link
Contributor

@ishitatsuyuki you can trigger a rebuilt by git push -f it also gives you the chance to rebase your branch so its up to date with master ~ ^^

@ishitatsuyuki
Copy link
Contributor Author

Two weeks elapsed. Please take a look!

@filipesilva
Copy link
Contributor

Heya @ishitatsuyuki, old system are used with the CLI, and we really don't want to introduce breaking changes for such little benefit. I'm sorry, but I'll have to close this PR.

@filipesilva filipesilva closed this May 8, 2017
@ishitatsuyuki
Copy link
Contributor Author

ishitatsuyuki commented May 8, 2017

@filipesilva I'm aware of such concerns. Actually, I'm willing to disable the removal of trailing slash (it can be specified as keepTrailingSlash, if it's going to keep compatibility with (really) old XHTML engines.

It's a tradeoff of compatibility with performance, though. If you reopened this, I will change it to allow XHTML ASAP.

@filipesilva
Copy link
Contributor

I don't quite get what the problem is that you're trying to solve though. Is there a bug currently that you're trying to fix?

@ishitatsuyuki
Copy link
Contributor Author

#1861

@filipesilva
Copy link
Contributor

Ah I see now! I'll reopen, and thank you for taking such an old issue on.

I'm slightly worried that it might affect some users that are currently depending on the current index.html format, but that's a matter for discussion. We're also thinking of adding a --minify flag so this could be controlled there for less disruption.

As for this PR as is, can you change it to not remove xhtml, and thus not change the other files? Your changes to packages/@angular/cli/tasks/eject.ts are just whitespace changes as well, and we normally don't allow that in PRs (it's just back and forth on file that don't need to change).

@filipesilva filipesilva reopened this May 9, 2017
@ishitatsuyuki
Copy link
Contributor Author

Looks like there's some presumably spurious build failures. Please trigger a rebuild and see if there is a real problem or not.

@clydin
Copy link
Member

clydin commented May 9, 2017

Some performance data if anyone is curious.

File size differences for a new project:

Minified Raw (bytes) Gzip (bytes)
No 701 377
Yes 674 365
Change -27 (~4%) -12 (~3%)

For a medium-size project with some additional index content:

Minified Raw (bytes) Gzip (bytes)
No 1163 550
Yes 1121 532
Change -42 (~4%) -18 (~3%)

@filipesilva filipesilva self-requested a review May 10, 2017 10:16
@filipesilva filipesilva self-assigned this May 10, 2017
@@ -37,7 +37,13 @@ export function getBrowserConfig(wco: WebpackConfigOptions) {
filename: path.resolve(buildOptions.outputPath, appConfig.index),
chunksSortMode: packageChunkSort(appConfig),
excludeChunks: lazyChunks,
xhtml: true
xhtml: true,
cache: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is cache set to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like a leftover from the inlining implementation. Will remove it.

@filipesilva
Copy link
Contributor

Restarted builds.

@clydin
Copy link
Member

clydin commented May 23, 2017

might want to adjust the commit message to indicate this just minifies index.html.

@ishitatsuyuki ishitatsuyuki changed the title Add support for minifying HTML Add support for minifying index.html May 24, 2017
@ishitatsuyuki
Copy link
Contributor Author

Review?

@clydin
Copy link
Member

clydin commented Jun 6, 2017

It looks like version 3.0 of the HtmlWebpackPlugin will remove built-in support for minification. Not sure on the timeline though.

@ishitatsuyuki
Copy link
Contributor Author

Semver will still protect us, and I believe someone will create a plugin to bring the functionality. If nobody do, maybe I will do that.

:( so slow review, it's going to enter the refactor cycle

@filipesilva filipesilva merged commit 75311c2 into angular:master Jun 7, 2017
@filipesilva
Copy link
Contributor

Sorry for the delay in getting it merged, I'm doing it now.

@icorne
Copy link

icorne commented Oct 31, 2018

Is there any way we can still get the trailing slash? Or to disable the minification of just the index.html?

@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 Sep 12, 2019
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.

6 participants