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

move uglify-js to dependencies #1392

Closed
wants to merge 1 commit into from

Conversation

kellyselden
Copy link

Closes #1391

@kellyselden kellyselden changed the base branch from master to 4.x October 11, 2017 16:37
@nknapp
Copy link
Collaborator

nknapp commented Oct 12, 2017

Personally, I'm in favour of keeping the dependency optional and handle the missing library case. It greatly reduces the amount of dependencies and downloaded megabytes if you don't need the precompiler. Opinions?

@rwjblue
Copy link
Contributor

rwjblue commented Oct 13, 2017

Agreed, but with the move to ES modules how do we conditionally import?

@nknapp
Copy link
Collaborator

nknapp commented Oct 13, 2017

I would make an execption here. It may make sense to move to completely towards es6-modules, but if they lack support for essential features, we should omit them when we can't use them.

I would suggest using require('uglify-js') instead and adding a lengthy comment, with the reason for not using es6.

@nknapp
Copy link
Collaborator

nknapp commented Oct 13, 2017

I made a PR here: #1394

@nknapp
Copy link
Collaborator

nknapp commented Oct 17, 2017

I'm closing this in favor of #1394

@nknapp nknapp closed this Oct 17, 2017
@kellyselden kellyselden deleted the uglify-js branch October 17, 2017 21:31
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.

3 participants