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

Fix/const to var #14384

Closed
wants to merge 2 commits into from
Closed

Fix/const to var #14384

wants to merge 2 commits into from

Conversation

mllustosa
Copy link

@mllustosa mllustosa commented Dec 11, 2018

This is related to the issue #14380. The latest update used const variable declaration which is not compatible with UglifyJS (UglifyJS doesn't support ES6). From what I saw, those were the only places where the library is using const and in all other places var is used, so I assumed that it was by mistake.

Of course that this is just a small draft and you guys know better what needs to be changed and where, but I thought that giving some sort of direction wouldn't hurt

I understand that:

  • I'm submitting this PR for reference only. It shows an example of what I'd like to see changed but
    I understand that it will not be merged and I will not be listed as a contributor on this project.

@clark-stevenson
Copy link

Thanks for this @mllustosa

I confirm that my webpack build is failing at the uglifyjs step because of it. I was in the middle of writing a stack overflow about why const was ending up in my output which I didn't expect.

@tagliala tagliala requested a review from robmadole December 12, 2018 19:01
@tagliala
Copy link
Member

@robmadole has this been fixed in 5.6.1?

@robmadole
Copy link
Member

Yeah we've fixed this in 5.6.1 👍

@robmadole robmadole closed this Dec 12, 2018
@tagliala
Copy link
Member

@mllustosa thanks for your PR

@mllustosa
Copy link
Author

@tagliala Anytime. Thanks for this awesome library and for the great and quick support 😄

@mllustosa mllustosa deleted the fix/const-to-var branch December 13, 2018 13:41
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.

4 participants