asmcrypto / asmcrypto.js

JavaScript Cryptographic Library with performance in mind.
MIT License
660 stars 182 forks source link

invalid asm.js (Firefox complains and rightfully so) #82

Closed ktarplee closed 9 years ago

ktarplee commented 9 years ago

http://asmjs.org/spec/latest/ states that asm.js should type parameters like so arg = arg|0;

In the source code this is fine. In the minified code it becomes compressed to arg |= 0 which Firefox claims is not valid asm.js with the error message

TypeError: asm.js type error: expecting argument type declaration for 'a' of the form 'arg = arg|0' or 'arg = +arg' or 'arg = fround(arg)'

Seems like the minified version is breaking asm.js. I turned off mangle and compress in the Gruntfile in uglify and the output was valid in Firefox.

vibornoff commented 9 years ago

It's well-known uglifyjs bug. I submitted a patch long time ago. Unfortunately @mishoo haven't let it in yet so I have to maintain patched version of uglifyjs until upstream update. Asmcrypto's package.json file states to install it from vibornoff/UglifyJS2.

Regarding to your report, it seems you have vanilla uglifyjs installed with --global option and your $PATH settings prefer system binary over patched one.

Check this from asmcrypto project root:

$ uglifyjs --version
$ node_modules/uglify-js/bin/uglifyjs --version
ktarplee commented 9 years ago

$ uglifyjs --version -bash: uglifyjs: command not found

$ node_modules/uglify-js/bin/uglifyjs --version util.puts: Use console.log instead uglify-js 2.4.999

So it seems I do not have a globally installed version of uglifyjs

ktarplee commented 9 years ago

I think the issue is that grunt-contrib-uglify includes uglify-js and that is the preferred version. I'm not completely sure how npm/node resolves node_modules but I think npm would have not installed that dependency if it didn't need it.

Specifically what I am talking about is the existence of: node_modules/grunt-contrib-uglify/node_modules/uglify-js I don't think it matters that the following exists node_modules/uglify-js node will prefer the grunt plugin's uglify-js. I could could be wrong however.

I am running the latest NPM and node which might be significant here.

ktarplee commented 9 years ago

I am now pretty sure that because npm installed uglify-js under grunt-contrib-uglify that the uglify plugin will use its own uglify instead of yours. See https://nodejs.org/api/modules.html

The issue then becomes why did npm feel the need to install a different version of uglify for the grunt plugin?

vibornoff commented 9 years ago

You're right, now I see my patched version of uglify is installed globally... That's why I never experienced this issue.

vibornoff commented 9 years ago

Heh, that's a common issue.

ktarplee commented 9 years ago

So it seems the solution is to use "npm shrinkwrap", right?

vibornoff commented 9 years ago

One does not simply "npm shrinkwrap" :)

It should be "npm dedupe && npm shrinkwrap". Buuuuut... asmcrypto depends on grunt-contrig-uglify ~0.8 which is resolved to 0.8.1 which depends on exact uglify-js =2.4.17 due to breaking changes in 2.4.18 whereas my patched version based on 2.4.20

ktarplee commented 9 years ago

Maybe you can use your uglify directly from grunt without the plugin? I have only used grunt plugins but I would not be surprised if you could just shell out to uglifyjs.

grunt.util.spawn({ cmd: ['rm'], args: ['-rf', '/tmp'], }, function done() { grunt.log.ok('/tmp deleted'); });

vibornoff commented 9 years ago

Should work now.

ktarplee commented 9 years ago

That does seem to work. Thank you.