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

The new minimize feature is causing Angular build to fail #269

Closed
JonWallsten opened this issue Mar 26, 2020 · 26 comments · Fixed by #304
Closed

The new minimize feature is causing Angular build to fail #269

JonWallsten opened this issue Mar 26, 2020 · 26 comments · Fixed by #304

Comments

@JonWallsten
Copy link

JonWallsten commented Mar 26, 2020

  • Operating System: Windows
  • Node Version: 12.14.1
  • NPM Version: 6.13.4
  • webpack Version: 4.42.1
  • html-loader Version: 4.0.1

Expected Behavior

The default configuration should not mess up the build for all Angular users

Actual Behavior

Since minimize is enabled as default for production builds it automatically fails Angular's production build when you update this package.

How Do We Reproduce?

Use Webpack and AngularCompilerPlugin to compile you Angular build.

I will close this as soon as it's created. I just want people to be able to search for this issue so they don't have to dig into this like I had to.

Solution

Turn of minimize:

{
    test: /\.html$/,
    loader: 'html-loader',
    options: {
        minimize: false
    }
},

Keywords: html-loader webpack angular build fail ivy compile

@alexander-akait
Copy link
Member

@JonWallsten What is a problem? Maybe we can fix it on loader side

@JonWallsten
Copy link
Author

@evilebottnawi I get hundreds of error, but they are all of the same kind.

NG8002: Can't bind to 'formcontrol' since it isn't a known property of 'input'
NG8002: Can't bind to 'inputvalue' since it isn't a known property of 'input-character-counter'
NG8003: No directive found with exportAs 'matMenuTrigger'

So something is happening with the template (i use external html files) that causes Angular to not being able to compile. I'm using their new compiler IVY which makes it even worse trying to figure out what's happening.

@alexander-akait
Copy link
Member

@JonWallsten Can you create reproducible test repo? I want to investigate and look what we can do

@JonWallsten
Copy link
Author

@JonWallsten Can you create reproducible test repo? I want to investigate and look what we can do

Sure! I'll send you an invite to a private repo as soon as it's done.

@alexander-akait
Copy link
Member

Great!

@chumbalum
Copy link

I'm having the same issue with angular prod builds. Went back to html-loader 0.5.5 for now

@JonWallsten
Copy link
Author

@evilebottnawi: You're invited now. I'll add instructions in an open issue in the repo.

@alexander-akait
Copy link
Member

@JonWallsten Thanks I will look at that tomorrow ⭐

@chumbalum
Copy link

chumbalum commented Mar 26, 2020

I found a solution for the problem. The console output in my application was

Error: Template parse errors:
Can't bind to 'ngif' since it isn't a known property of 'span'. ("<span class="spinner" [ERROR ->]*ngif="loading">

I noticed that ngif is mentioned in small case

So the following config of html-minifier-terser did the trick

test: /\.html$/,
use: [{
	loader: "html-loader",
	options: {minimize: {caseSensitive: true}}
}]

The caseSensitive option is false by default. I also experimented with the customAttrAssign option but as far as I can tell, all angular attributes are working out of the box.

Edit:
The option removeAttributeQuotes also seems to be a problem. So with the following config, my compilation succeeded

use: [{
	loader: "html-loader",
	options: {
		minimize: {
			caseSensitive: true,
			collapseWhitespace: true,
			conservativeCollapse: true,
			keepClosingSlash: true,
			minifyCSS: true,
			minifyJS: true,
			removeAttributeQuotes: false,
			removeComments: true,
			removeScriptTypeAttributes: true,
			removeStyleLinkTypeAttributes: true,
			useShortDoctype: true
		}
	}
}]

(since removeAttributeQuotes is false by default, the option could be removed in https://github.com/webpack-contrib/html-loader/blob/master/src/plugins/minimizer-plugin.js)

@JonWallsten
Copy link
Author

@chumbalum: Nice find! Now I feel embarrassed I didn't even take the time to realize my own template variables were all in lowercase in the error messages.

@alexander-akait
Copy link
Member

@chumbalum Thanks for investigation, maybe we can detect what it is angular template and uses other settings for minimizer, if not we should documented that

@dreyks
Copy link

dreyks commented Apr 13, 2020

I really think html loader should not be responsible for the minification. or at least this should be opt-in, not opt-out

I'm using html-loader in a pipeline, so that other loaders come after it. During the minify phase html-loader removed attribute quotes because the values did not have any spaces in them but after the next loader (which is handlebars-loader) expanded the variables the html became broken

Moreover, since minification by default only happens in production, the issue was only caught on CI (good thing we run them in prod mode, which as i know not everybody does)

UPD: I know about the preprocess option but i can't use it since i'm using handlebars with marionette and marionette expects the template to be a function and not a string

@alexander-akait
Copy link
Member

@dreyks we already use minimizer in production mode - for js/css/html/etc, it is good DX to set mode: "production" and forget about other options in the config.

During the minify phase html-loader removed attribute quotes because the values did not have any spaces in them but after the next loader (which is handlebars-loader) expanded the variables the html became broken

Please procide reproducible test repo, I will think how we can improve it

@Zadvornyi
Copy link

@chumbalum Thanks for your solution, this example works for me (with removeAttributeQuotes: true):

            {
                test: /\.html$/,
                use: [{
                    loader: "html-loader",
                    options: {
                        minimize: {
                            caseSensitive: true,
                            collapseWhitespace: true,
                            conservativeCollapse: true,
                            keepClosingSlash: true,
                            minifyCSS: true,
                            minifyJS: true,
                            removeAttributeQuotes: true,
                            removeComments: true,
                            removeScriptTypeAttributes: true,
                            removeStyleLinkTypeAttributes: true,
                            useShortDoctype: true
                        }
                    }
                }],
                exclude: /node_modules/
            },

@alexander-akait
Copy link
Member

@Zadvornyi Can you create reproducible test repo? I want to fix it on html-loader side

@stnor
Copy link

stnor commented Jul 17, 2020

The config above doesn't work for me on Angular 10 (without aot)... :(
Was using 1.1.0.
0.5.5 works.

Uncaught Error: Template parse errors:
Unexpected closing tag "a". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags ("<div class=masthead> <a href=/ > <div class=logo></div> [ERROR ->]</a> <nav class=nav> <section *ngIf=isNativeAnonymous()> <button class="button secondary-button" mat-"): ng:///Th/Oh.html@0:56
Unexpected closing tag "a". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags ("secondary-button" href=/nomp-plus> <i class="far fa-external-link"></i>&nbsp;<span>Nomp Plus</span> [ERROR ->]</a> </section> <section *ngIf=!principal> <a *ngIf=!isNativeApp() href=# (click)="changeLanguage('en"): ng:///Th/Oh.html@0:793

@alexander-akait
Copy link
Member

@stnor Can you provide minimum reproducible test?

@stnor
Copy link

stnor commented Jul 21, 2020

Zip file with minimum reproducible test (I don't use AngularCompilerPlugin yet as I have a large legacy app that's hybrid ng1 and ng10).

test-case.zip

my test case looked like this:

<div>
       <h1>hello</h1>
       <a href="/">link</a>
</div>
            {
                test: /\.html$/,
                use: [{
                    loader: "html-loader",
                    options: {
                        minimize: {
                            caseSensitive: true,
                            collapseWhitespace: true,
                            conservativeCollapse: true,
                            keepClosingSlash: true,
                            minifyCSS: true,
                            minifyJS: true,
                            removeAttributeQuotes: false,  <---- fixed it, tldr
                            removeComments: true,
                            removeScriptTypeAttributes: true,
                            removeStyleLinkTypeAttributes: true,
                            useShortDoctype: true
                        }
                    }
                }]
            },`

@alexander-akait
Copy link
Member

Thanks!

@stnor
Copy link

stnor commented Jul 22, 2020

So I guess <a href="/">foo</a becomes <a href=/>foo</a>which causes it to be invalid HTML as the a tag now is closed twice.

@alexander-akait
Copy link
Member

@alexander-akait
Copy link
Member

Can you open an issue?

@alexander-akait
Copy link
Member

alexander-akait commented Jul 22, 2020

Problem with caseSensitive is on html-minifier-terser side, too, because we should not touch unknown attributes, like ngIf

I think we can set caseSensitive: true and removeAttributeQuotes: false by default, until it was fixed

What do you think?

@stnor
Copy link

stnor commented Jul 30, 2020

@evilebottnawi I think it is probably better if you open an issue with html-minifier-terser. I have moved on to more pressing matters.

Your proposed solution for html-loader sounds good to me.

@alexander-akait
Copy link
Member

Somebody can provide spec on ivy? Need investigate, I found only https://github.com/angular/angular/blob/master/packages/core/src/render3/STATUS.md

@alexander-akait
Copy link
Member

And why html-loader uses on angular ivy tempaltes?

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.

6 participants