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

Add support for the Calmjs javascript minifier #957

Merged
merged 12 commits into from
Oct 12, 2019

Conversation

alvra
Copy link
Contributor

@alvra alvra commented Oct 12, 2019

Calmjs is a javascript minifier similar to SlimIt that has some advantages. https://github.com/calmjs/calmjs.parse
For me, it solves the following problem:
rspivak/slimit#97

@diox
Copy link
Member

diox commented Oct 12, 2019

Hi,

Sorry, but I think this doesn't merit inclusion in django-compressor core. You can use it in your own project if needed as it doesn't require anything special on compressor's side to do that.

@diox diox closed this Oct 12, 2019
@alvra
Copy link
Contributor Author

alvra commented Oct 12, 2019

SlimIt hasn't seen a new release since 2013, which means right now, there are a number of problems with it. For example, pretty much every feature of ES6 is unsupported. Pull requests for these features have been waiting since as far back as 2013.

Meanwhile Calmjs' last release was a few months ago, ES6 is partially supported, it can generate source maps, and it's slightly faster than SlimIt.

If Django-Compressor is to provide a single js minifier out of the box, I don't think SlimIt is the right choice anymore IMO. We should keep it for backwards compatibility, but please consider adding a better option like Calmjs.

@diox
Copy link
Member

diox commented Oct 12, 2019

Thanks for your reply, I appreciate it. I don't really like shipping too many filters by default, especially relatively trivial ones, but I you make a good point about slimit not cutting it any more.

@diox diox reopened this Oct 12, 2019
@diox
Copy link
Member

diox commented Oct 12, 2019

One thing I don't like about the slimit integration, and this one has the same problem, is that there isn't even a basic test covering it. Could you add the latest calmjs version in requirements/tests.txt and add a test making sure it works? You can draw inspiration from the JsMinTestCase one in compressor/tests/test_filters.py.

@alvra
Copy link
Contributor Author

alvra commented Oct 12, 2019

Thanks! It bothered me too, so I added tests for both SlimIT and Calmjs.

@diox diox merged commit a5bfd77 into django-compressor:develop Oct 12, 2019
@diox
Copy link
Member

diox commented Oct 12, 2019

Thank you for your contribution! I'll do a release eventually later this month, in the meantime it'll be available on the develop branch.

@alvra
Copy link
Contributor Author

alvra commented Oct 12, 2019

Great, thanks!

@albertyw albertyw mentioned this pull request Dec 3, 2019
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