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

Minification not working #2055

Closed
kireerik opened this issue Jun 5, 2017 · 19 comments · Fixed by #2068
Closed

Minification not working #2055

kireerik opened this issue Jun 5, 2017 · 19 comments · Fixed by #2068
Labels

Comments

@kireerik
Copy link

kireerik commented Jun 5, 2017

kangax/html-minifier#823

ES5 in and out put example:

window.fbAsyncInit = function() {
	FB.init({
		version: 'v2.9'
	})
}

Uglify version: 3.0.x

minify() options used: The one's that HTMLMinifier uses

@alexlamsl
Copy link
Collaborator

Your example seems to be working as expected:

$ cat test.js
window.fbAsyncInit = function() {
        FB.init({
                version: 'v2.9'
        })
}

$ uglifyjs test.js -mc
window.fbAsyncInit=function(){FB.init({version:"v2.9"})};

@alexlamsl
Copy link
Collaborator

Even if I try to use html-minifier online it still seems to work as expected:

<html>
<head>
<script>
window.fbAsyncInit = function() {
        FB.init({
                version: 'v2.9'
        })
}
</script>
</head>
<body></body>
</html>

using default options:

<script>window.fbAsyncInit=function(){FB.init({version:"v2.9"})}</script>

@kireerik
Copy link
Author

kireerik commented Jun 5, 2017

I see. I have also tried HTMLMinifier's online version and it worked for me as well.

I have tried this in my example project and it works, even if I upgrade it to HTMLMinifier 3.5.2. So unfortunately I can't reproduce this issue in that project.

However in my private project, which is based on the mentioned example project, it doesn't work. I investigated the issue and it was definitely intruduced when the project's HTMLMinifier dependency was updated to version 3.5.2 from version 3.4.4 (the yarn upgrade command was used and only the .yarn lock file was changed in that commit).
I can add you to my private repository on GitLab. On that project this issue can be reproduced consistently by toggling HTMLMinifier's version.

@alexlamsl
Copy link
Collaborator

I don't have an account on GitLab unfortunately, nor am I too familiar to yarn.

Do you use html-minifier directly or via some other toolings?

@alexlamsl
Copy link
Collaborator

I'd also look at the equivalent of npm ls to see if you have a single copy of uglify-js (and presumably html-minifier) - if there is say another copy of ugliy-js 2.x around then it may confuse require("uglify-js")

@kireerik
Copy link
Author

kireerik commented Jun 5, 2017

I see. You can register here if you wish (for free). It is really similar to GitHub. Basically I am using yarn for performance reasons only. By the way it is also really similar to npm on the high level.

Yes, I am using it directly as you can see here.

Thank you for your suggestion!

I have 1 copy of html-minifier, but I have several copies of uglify-js as you can see here:

  • html-minifier@3.5.2
  • uglify-js@3.0.15
  • webpack@2.6.1
    • uglify-js@^2.8.27
    • uglify-js@2.8.28

Somehow I have fewer uglify-js copies with the older html-minifier version:

  • html-minifier@3.4.4
    • uglify-js@~2.8.22
  • uglify-js@2.8.28
  • webpack@2.6.1
    • uglify-js@^2.8.27

@alexlamsl
Copy link
Collaborator

Does your codebase run to some extent without webpack? If so, you can temporary remove it (alongside uglify-js 2.8.28) and see if that makes html-minifier work.

@kireerik
Copy link
Author

kireerik commented Jun 5, 2017

Yes, it does, both in production and development mode. I have removed webpack manually once the server was running, but the issue still persist.

The build process is handled by Razzle which is using webpack under the hood.

@alexlamsl
Copy link
Collaborator

alexlamsl commented Jun 5, 2017

I think you need to remove that copy of uglify-js 2.x before any code is run, because otherwise it would have imported the older module into memory already.

Other than that, I'm out of ideas for now - will post if something comes to mind. Meanwhile if you can narrow it down to a test case that would be helpful as well.

@kireerik
Copy link
Author

kireerik commented Jun 6, 2017

I see. Unfortunately this doesn't helps either. I have removed them before actually starting the server after the build process.

The only thing I have noticed is that html-minifier is trying to use uglify-js from the root of the node_modules folder and not from it's own node_modules folder (it is only linked there), because if I delete uglify-js from the root node_modules folder than the server couldn't start, because node_modules\html-minifier\src\htmlminifier.js is missing the uglify-js module.

@alexlamsl
Copy link
Collaborator

Yes, even npm would try and flatten the dependency hierarchy these days.

I guess you might have done it already - have you tried a clean npm install, i.e. not using yarn and see if it fails the same way? By "clean" this is what I normally do:

  • nuke the node_modules folder
  • npm cache clean (or with npm@5, npm cache clean --force)
  • npm install --no-optional --no-save

@kireerik
Copy link
Author

kireerik commented Jun 6, 2017

I see, I also thought it is fine.

Yes, I have. It seems you know me well. :)

Now I have tried it with using npm cache clean as well and it is not working in the same way.

What do you think, should I raise an issue at Razzle?

@alexlamsl
Copy link
Collaborator

What do you think, should I raise an issue at Razzle?

Worth a shot - I'm out of ideas at the moment. 😅

@kireerik
Copy link
Author

kireerik commented Jun 7, 2017

Finally I have found what causing this. So the issue became reproducible.

This issue happens, when you use HTMLMinifier 3.5.0+ and you add the following code or similiar codes to this line:

Array.prototype.contains = function(element) {
	return !!~this.indexOf(element)
}

Plus you add a code like this to the server somewhere here:

<script>
	window.fbAsyncInit = function() {
		FB.init({
			version: 'v2.9'
		})
	}
</script>

So now this issue can be reproduced with my Razzle Material UI Styled Example repository.

Note: HTML5Minifier's version should be changed as well here.

@alexlamsl
Copy link
Collaborator

Thanks for your perseverance - I shall look into your example above and investigate right away 😉

@kireerik
Copy link
Author

kireerik commented Jun 7, 2017

You are welcome! All right, thank you!

@alexlamsl
Copy link
Collaborator

So preliminary results show that by overriding Array.prototype.contains, you have managed to kill require("uglify-js").minify():

--- a/package.json
+++ b/package.json
@@ -6,7 +6,7 @@
        , "dependencies": {
                "express": "4.x"
                , "compression": "1.x"
-               , "html-minifier": "3.4.4"
+               , "html-minifier": "3.5.2"

                , "react": "15.x"
                , "react-dom": "15.x"
--- a/src/application/Main.js
+++ b/src/application/Main.js
@@ -21,7 +21,13 @@ const Div = styled.div`
        text-align: center;
        padding-top: 200px;
 `
-
+Array.prototype.contains = function(element) {
+       try {
+               return !!~this.indexOf(element)
+       } catch (e) {
+               console.error(e.stack)
+               throw e
+       }
+}
 class Main extends Component {
        constructor(properties, context) {
                super(properties, context)
--- a/src/server.js
+++ b/src/server.js
@@ -37,8 +37,13 @@ server
                                        ` + (assets.client.css ?
                                                '<link rel="stylesheet" href="' + assets.client.css + '">' : ''
                                        ) + css + `
-
-                                       <script async src="` + assets.client.js + `"></script>
+<script>
+       window.fbAsyncInit = function() {
+               FB.init({
+                       version: 'v2.9'
+               })
+       }
+</script>
                                </head>
                                <body>
                                        <div id="root">` + html + `</div>

This will give, after npm start and hitting http://localhost/:

TypeError: this.indexOf is not a function
    at Object.__dirname../src/application/Main.js.Array.contains [as input] (razzle-material-ui-styled-example/build/server.js:135:18)
    at next (eval at <anonymous> (razzle-material-ui-styled-example/node_modules/uglify-js/tools/node.js:21:1), <anonymous>:2036:25)
    at parse (eval at <anonymous> (razzle-material-ui-styled-example/node_modules/uglify-js/tools/node.js:21:1), <anonymous>:2022:15)
    at Object.minify (eval at <anonymous> (razzle-material-ui-styled-example/node_modules/uglify-js/tools/node.js:21:1), <anonymous>:10444:42)
    at Object.options.minifyJS (razzle-material-ui-styled-example/node_modules/html-minifier/src/htmlminifier.js:682:29)
    at Object.chars (razzle-material-ui-styled-example/node_modules/html-minifier/src/htmlminifier.js:1164:24)
    at razzle-material-ui-styled-example/node_modules/html-minifier/src/htmlparser.js:225:19
    at String.replace (<anonymous>)
    at new HTMLParser (razzle-material-ui-styled-example/node_modules/html-minifier/src/htmlparser.js:217:19)
    at minify (razzle-material-ui-styled-example/node_modules/html-minifier/src/htmlminifier.js:946:3)

html-minifier has a self-defence mechanism which returns the script code verbatim if uglify-js fails:
https://github.com/kangax/html-minifier/blob/9a73a4de11b798e5f79b0a8ee9d7573f1dc570f9/src/htmlminifier.js#L683-L686
... which should explain what you have reported thus far.

Technically it is out of scope for uglify-js (or any standard JS libraries for that matter) to remain functional when built-in functions are redefined. Though I'm now very curious because a quick search against uglify-js codebase does not turn up any usage of Array.prototype.contains, so I'll have a deeper look and report back.

@kireerik
Copy link
Author

kireerik commented Jun 7, 2017

I see.

I don't overwrite Array.prototype.contains. I am creating it instead. Array.prototype.contains doesn't exists by default, because it is not a built-in function and so it is undefined by default.

I have tried your example and you can name the function whatever you wish and the same error will be displayed. For example: Array.prototype.myContains or Array.prototype.whatever.

@alexlamsl
Copy link
Collaborator

Indeed - and I think I know why now:
https://github.com/mishoo/UglifyJS2/blob/793d61499b4ab53cdfdd3b81b9faf9f36b77bae8/lib/minify.js#L89-L97

Basically it's down to this:

$ node
> for (var name in ['a','b']) console.log(name);
0
1

> Array.prototype.contains = 'foo';
> for (var name in ['a','b']) console.log(name);
0
1
contains

So this is a bug with lib/minify.js - I shall proceed to fix now. Thanks for helping out! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants