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

UglifyJS compressing causes Range Error #60

Merged
merged 1 commit into from
Jan 8, 2013
Merged

UglifyJS compressing causes Range Error #60

merged 1 commit into from
Jan 8, 2013

Conversation

fidian
Copy link
Contributor

@fidian fidian commented Jan 7, 2013

UglifyJS2 with compression enabled and default options will see the line

"" + this.toString();

and minify it into this line

"" + this

and that normally should work, but the .valueOf() function then falls into
an infinite loop. Assigning the same function to both .toString() and
.valueOf() not only eliminates this problem, but it also shrinks the code an
eensy weensy bit.

UglifyJS2 with compression enabled and default options will see the line
    "" + this.toString()
and minify it into this line
	"" + this
and that normally should work, but the .valueOf() function then falls into
an infinite loop.  Assigning the same function to both .toString() and
.valueOf() not only eliminates this problem, but it also shrinks the code an
eensy weensy bit.
@rodneyrehm
Copy link
Member

looks good! don't even remember why that used to be two separate functions.

@fidian
Copy link
Contributor Author

fidian commented Jan 8, 2013

What is the process for getting a pull request merged? Is there a wait time or a review checklist? Mandatory quarantine period? :-) I ask only because I'd love to update the git submodule in my project instead of pointing everyone on the team temporarily to my fork.

rodneyrehm added a commit that referenced this pull request Jan 8, 2013
UglifyJS compressing causes Range Error
@rodneyrehm rodneyrehm merged commit cc78e29 into medialize:gh-pages Jan 8, 2013
@rodneyrehm
Copy link
Member

merged and released :)
(sorry, having a busy week…)

@fidian
Copy link
Contributor Author

fidian commented Jan 9, 2013

Fantastic! Thanks for getting that rolled out.
On Jan 8, 2013 5:37 PM, "Rodney Rehm" notifications@github.com wrote:

merged and released :)
(sorry, having a busy week…)


Reply to this email directly or view it on GitHubhttps://github.com//pull/60#issuecomment-12023658.

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 this pull request may close these issues.

2 participants