feat: abstract minify and use value for all modes #3965

manniL · 2018-09-25T18:48:03Z

Types of changes

New feature (non-breaking change which adds functionality)

Description

Currently, we use generate.minify for configuring the html-minifier in generate mode.
With this PR we can use build.minify for all modes to fine-tune the minifier.

The value of build.minify will fall back to the old values in the respective modes to avoid a breaking change.

Resolves #3961, resolves #3964

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly. (PR: docs(build): add changes concerning minify and html.minify docs#799)
I have added tests to cover my changes (if not applicable, please state why)
All new and existing tests passed.

clarkdo

Another thought is that if build.minify has some confusion with build.optimization.minifier?
How about htmlMinify or some name else?

lib/common/options.js

clarkdo · 2018-09-25T19:02:04Z

lib/builder/generator.js

+    // Legacy workaround for obsolete options.generate.minify
+
+    if (typeof this.options.build.minify === 'undefined') {
+      this.options.build.minify = {


I think this can be default value of build.minify directly.

The problem is that we'd possibly introduce a breaking change because this value is not equal to minify: true or the resulting default values (which was the value for SSR/SPA layout minification).

I think there actually no difference to users because they don’t need change anything and also wont influence their project features, it’s just some specific options of minifier, let’s ask @pi0 and @Atinux opinion.

In the past things like script injection broke because of minification (where closing tags were removed), so I can't judge which problems the changed setting might cause. 🙈

But it shouldn't be up to me ☺️

Yep, but current default options are more conservative than true, right? It should only fix sth instead of breaking 😅

Ah! No, they are not. In this situation I completely agree!

clarkdo · 2018-09-25T19:03:00Z

lib/builder/webpack/client.js

@@ -47,13 +47,19 @@ export default class WebpackClientConfig extends WebpackBaseConfig {
  plugins() {
    const plugins = super.plugins()

+    // Legacy workaround for obsolete options.generate.minify
+
+    if (typeof this.options.build.minify === 'undefined') {


If we make above value as deault value, this part can be removed

manniL · 2018-09-25T19:10:23Z

I agree on changing the name to htmlMinify! That fits better and doesn't cause as much confusion

galvez

@manniL just need to fix the unit test, otherwise LGTM

lib/common/options.js

lib/builder/generator.js

lib/builder/webpack/client.js

codecov-io · 2018-09-25T19:21:49Z

Codecov Report

Merging #3965 into dev will decrease coverage by 0.15%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##              dev    #3965      +/-   ##
==========================================
- Coverage   97.56%   97.41%   -0.16%     
==========================================
  Files          18       18              
  Lines        1233     1237       +4     
  Branches      337      338       +1     
==========================================
+ Hits         1203     1205       +2     
- Misses         29       31       +2     
  Partials        1        1

Impacted Files	Coverage Δ
lib/builder/webpack/client.js	`96.87% <ø> (ø)`	⬆️
lib/common/nuxt.config.js	`100% <ø> (ø)`	⬆️
lib/builder/generator.js	`91.34% <66.66%> (-1.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c1d94a...23cc12e. Read the comment docs.

manniL · 2018-09-25T19:26:39Z

I'll move all options-related stuff to options.js and will report back then.

manniL · 2018-09-25T20:49:58Z

Refactored the changes to use the former generate.minify as default value for build.htmlMinify and added a warning concerning overrides

clarkdo · 2018-09-25T20:55:20Z

lib/common/options.js

@@ -209,6 +209,14 @@ Options.from = function (_options) {
    delete options.render.gzip
  }

+  // Legacy: Override build.htmlMinify with generate.minify if present
+  if (this.options.generate.minify) {
+    this.options.build.htmlMinify = this.options.generate.minify


This part still have the issue we just discussed, I think it should only be executed in generation

But how can we distinguish between generated and "normal" SSR?

Unfortunately we can’t, so maybe just warning default value willl be applied but not override build options? @pi0 How do you think?

That would be a SOFT BREAKING CHANGE :D Breaking change but warn to fix it :D But i agree.

That's why I was unsure if we should really proceed this way. Also because can't oversee how much damage the new "defaults" can cause for SSR HTML minification 🤔

manniL · 2018-09-25T21:40:02Z

To avoid breaking changes, the minification settings will be overridden in the generator module when generate.minify is present (but not in options).

We can then introduce the minification improvements that are set as default values of build.htmlMinify in 2.1 without risking a breaking change.

clarkdo · 2018-09-25T21:53:43Z

lib/builder/generator.js

    if (this.options.generate.minify) {
+      minificationOption = this.options.generate.minify
+      consola.warn('generate.minify has been deprecated. Use build.htmlMinify instead!')


If we all agree to do this way, I suggest adding a message that this option will be removed in 2.1

You mean generate.minify? Though it's not super clean, I wouldn't remove the override in the next minor version. Otherwise, we don't follow semver.

Yeah, what I mean is just making warning message more clear

Would ...deprecated and will be removed in the next major version be sufficient?

manniL · 2018-09-25T23:44:52Z

I have no clue why the CI fails. Will investigate later on.
If anybody has an idea or so, feel free!

Update: I have a slight feeling that the old generator.minify settings will not work for SSR/SPA. 🤔

Update 2: My feeling was right 🙈

stale

manniL · 2018-09-27T09:30:22Z

I had to remove sortAttributes from the list of defaults because it broke the normal templates.
I'm confused though: Does minification happen before or after injecting the template (or both)? I've tried to trace it down and saw replacing the template happening quite early. Unfortunately hadn't time to dig in deeper.

Atinux

Good for me, could you add a PR to nuxt/docs?

manniL · 2018-09-28T16:39:24Z

Will add a doc PR, yup!

manniL · 2018-09-28T17:23:12Z

@Atinux PR added and linked.

lock · 2018-10-31T17:39:15Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

feat: abstract minify and use value for all modes

5636470

manniL requested review from pi0, clarkdo, Atinux, galvez and aldarund September 25, 2018 18:49

clarkdo reviewed Sep 25, 2018

View reviewed changes

galvez approved these changes Sep 25, 2018

View reviewed changes

change name to htmlMinify

3cbbfda

pi0 previously requested changes Sep 25, 2018

View reviewed changes

lib/common/options.js Outdated Show resolved Hide resolved

lib/builder/generator.js Outdated Show resolved Hide resolved

lib/builder/generator.js Outdated Show resolved Hide resolved

lib/builder/webpack/client.js Outdated Show resolved Hide resolved

only override htmlMinify with legacy value in generate mode

56cbb7c

galvez assigned manniL Sep 25, 2018

Use default options for htmlMinify and add override info to warning

ef5fb45

manniL force-pushed the feat-abstract-html-minifier branch from 682fb33 to ef5fb45 Compare September 25, 2018 20:55

clarkdo reviewed Sep 25, 2018

View reviewed changes

manniL added 2 commits September 25, 2018 22:36

Remove redundant minification settings

bf9ebaf

Override minification settings in generator

84dfb10

Cleanup and add TODO for nuxt 3

d43a6a5

clarkdo reviewed Sep 25, 2018

View reviewed changes

Improve warning

fb4639d

manniL added 3 commits September 26, 2018 20:56

set default minify option to "true"

c01e925

make tests pass again

e1e0c69

remove the culprit

e5ad9d9

manniL requested review from pi0 and clarkdo September 26, 2018 20:58

replace htmlMinify with html.minify

a34b060

pi0 approved these changes Sep 27, 2018

View reviewed changes

Merge branch 'dev' into feat-abstract-html-minifier

0ea8a69

aldarund approved these changes Sep 27, 2018

View reviewed changes

Merge branch 'dev' into feat-abstract-html-minifier

23cc12e

Atinux approved these changes Sep 28, 2018

View reviewed changes

manniL mentioned this pull request Sep 28, 2018

docs(build): add changes concerning minify and html.minify nuxt/docs#799

Merged

Atinux merged commit d69b4b8 into dev Sep 30, 2018

Atinux deleted the feat-abstract-html-minifier branch September 30, 2018 16:13

lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018

danielroe added the 2.x label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: abstract minify and use value for all modes #3965

feat: abstract minify and use value for all modes #3965

manniL commented Sep 25, 2018 •

edited

clarkdo left a comment

clarkdo Sep 25, 2018

manniL Sep 25, 2018

clarkdo Sep 25, 2018

manniL Sep 25, 2018

clarkdo Sep 25, 2018

manniL Sep 25, 2018

clarkdo Sep 25, 2018

manniL commented Sep 25, 2018

galvez left a comment

codecov-io commented Sep 25, 2018 •

edited

manniL commented Sep 25, 2018

manniL commented Sep 25, 2018

clarkdo Sep 25, 2018

manniL Sep 25, 2018

clarkdo Sep 25, 2018

pi0 Sep 25, 2018

manniL Sep 25, 2018

manniL commented Sep 25, 2018

clarkdo Sep 25, 2018

manniL Sep 25, 2018

clarkdo Sep 25, 2018

manniL Sep 25, 2018

clarkdo Sep 25, 2018

manniL commented Sep 25, 2018 •

edited

manniL commented Sep 27, 2018

Atinux left a comment

manniL commented Sep 28, 2018

manniL commented Sep 28, 2018

lock bot commented Oct 31, 2018

feat: abstract minify and use value for all modes #3965

feat: abstract minify and use value for all modes #3965

Conversation

manniL commented Sep 25, 2018 • edited

Types of changes

Description

Checklist:

clarkdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manniL commented Sep 25, 2018

galvez left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 25, 2018 • edited

Codecov Report

manniL commented Sep 25, 2018

manniL commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manniL commented Sep 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manniL commented Sep 25, 2018 • edited

manniL commented Sep 27, 2018

Atinux left a comment

Choose a reason for hiding this comment

manniL commented Sep 28, 2018

manniL commented Sep 28, 2018

lock bot commented Oct 31, 2018

manniL commented Sep 25, 2018 •

edited

codecov-io commented Sep 25, 2018 •

edited

manniL commented Sep 25, 2018 •

edited