Skip to content

Fix problem when minifying with Uglify2 #110

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix problem when minifying with Uglify2 #110

wants to merge 2 commits into from

Conversation

pjeweb
Copy link

@pjeweb pjeweb commented Nov 17, 2015

Code did not work because basically [1, undefined, 2].join('') gets minified to 1 + undefined + 2 in Uglify2 with compress.evaluate option set to true.
Also make code better understandable :)

Tested with https://skalman.github.io/UglifyJS-online/:

function a() {
  return (document.cookie = [
    key, '=', value,
    attributes.expires && '; expires=' + attributes.expires.toUTCString(), // use expires attribute, max-age is not supported by IE
    attributes.path    && '; path=' + attributes.path,
    attributes.domain  && '; domain=' + attributes.domain,
    attributes.secure ? '; secure' : ''
  ].join(''));
}
function a(){return document.cookie=key+"="+value+(attributes.expires&&"; expires="+attributes.expires.toUTCString())+(attributes.path&&"; path="+attributes.path)+(attributes.domain&&"; domain="+attributes.domain)+(attributes.secure?"; secure":"")}

@pjeweb
Fix problem when minifying with Uglify2
7b9cf25 
Code did not work because `[1, undefined, 2].join('')` gets minified to `1 + undefined + 2` in Uglify2 with compress.evaluate option set to true.
Also make code better understandable :)
@pjeweb
Fixed typo
3b7c3cb 
copy paste failed here ;)
@FagnerMartinsBrack
Copy link
Member

I will take a look on it later, but it seems to be a problem with Uglify2.

@pjeweb
Copy link
Author

pjeweb commented Nov 18, 2015

Yes it's UglifyJS's problem but still this code relies on a not easily understandable feature of JS.
Another option would be to use the foo ? 'bar' : ''; syntax as it is used for attributes.secure already, preventing undefined in the array. This way you don't need to change as much. It stays compact while being consistent :)

@FagnerMartinsBrack
Copy link
Member

this code relies on a not easily understandable feature of JS

That's a trade-off that was made in order to maintain the "~800 bytes gzipped" bullet point documented in the "Features" section of the README. Right now we have 874 bytes gzipped as from #71.

I understand the legibility argument and I agree with that. The point is that we have currently 100% test coverage and use TDD extensively, so it is unlikely that a bug arises from now on because of those legibility problems.

Feel free to throw a grunt compare-size comparison. If it cut a single byte and there is no external implications, then I will be 👍 to merge it

@pjeweb pjeweb mentioned this pull request Nov 19, 2015
Closed
@pjeweb
Copy link
Author

pjeweb commented Nov 19, 2015

There is apparently a pull request for that issue in Uglify from 2013 (mishoo/UglifyJS#301). Hope it gets fixed there. When I have the time I will look into grunt compare-size though.

@FagnerMartinsBrack
Copy link
Member

In the mean time you can use the minified version from the Releases page or build your own minified file using:

$ grunt

@pjeweb
Copy link
Author

pjeweb commented Nov 20, 2015

Well we have a running build process (using gulp, babel, browserify) and integrating another build process into that is not really worth it.
But as a comment on the issue (mishoo/UglifyJS#301 (comment)) on Uglify noted it was due to an unsafe option that I somehow enabled in all my tests :)

@pjeweb pjeweb closed this Nov 20, 2015
@pjeweb
Copy link
Author

pjeweb commented Nov 20, 2015

Btw thanks for responding fast and this project 👍

@FagnerMartinsBrack
Copy link
Member

You're welcome. Good hacking 👍

@FagnerMartinsBrack FagnerMartinsBrack added this to the v2.1.0 milestone Dec 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
v2.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@pjeweb @FagnerMartinsBrack