Skip to content

Fix problem when minifying with Uglify2#110

Closed
pjeweb wants to merge 2 commits intojs-cookie:masterfrom
pjeweb:master
Closed

Fix problem when minifying with Uglify2#110
pjeweb wants to merge 2 commits intojs-cookie:masterfrom
pjeweb:master

Conversation

@pjeweb
Copy link
Copy Markdown

@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
Copy Markdown
Member

FagnerMartinsBrack commented Nov 17, 2015

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

@pjeweb
Copy link
Copy Markdown
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
Copy Markdown
Member

FagnerMartinsBrack commented Nov 18, 2015

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
Copy Markdown
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
Copy Markdown
Member

FagnerMartinsBrack commented Nov 19, 2015

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
Copy Markdown
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
Copy Markdown
Author

pjeweb commented Nov 20, 2015

Btw thanks for responding fast and this project 👍

@FagnerMartinsBrack
Copy link
Copy Markdown
Member

FagnerMartinsBrack commented Nov 20, 2015

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