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

use JShrink instead of JSMin (#13052) #13185

Closed
wants to merge 1 commit into from

Conversation

AdamWill
Copy link
Contributor

@AdamWill AdamWill commented Jan 9, 2015

The JSMin minifier is non-free. JShrink is free, it's also a currently-maintained project following good development practices (though we may wish to switch to JSqueeze soon for performance reasons; waiting on
kriswallsmith/assetic#698 reaching upstream Assetic 1.2 for that). This goes along with owncloud-archive/3rdparty#149 , a 3rdparty PR that drops mrclay/minify and adds JShrink.

NOTE: this will fail without the matching 3rdparty commit, as JShrink will not be available.

The JSMin minifier is non-free. JShrink is free, it's also a
currently-maintained project following good development
practices (though we may wish to switch to JSqueeze soon for
performance reasons; waiting on
kriswallsmith/assetic#698 reaching
upstream Assetic 1.2 for that). This goes along with a 3rdparty
commit that drops mrclay/minify and adds JShrink.
@ghost
Copy link

ghost commented Jan 9, 2015

Thanks a lot for your contribution! Contributions to the core repo require a signed contributors agreement http://owncloud.org/contribute/agreement/ Alternatively you can add a comment here stating that this contribution is MIT licensed. Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@DeepDiver1975
Copy link
Member

JShrinkFilter is missing - we could add this right next to SeparatorFilter in lib/private/assetic

@AdamWill
Copy link
Contributor Author

AdamWill commented Jan 9, 2015

Gah, you're right - I'd forgotten about that in the damn shrinker merry-go-round. If we have to twiddle Assetic anyway we may as well just go straight to JSqueeze, I guess? I'll send a different PR in a bit.

@AdamWill
Copy link
Contributor Author

#13229 is an alternative which uses JSqueeze instead of JShrink; as we'd have to mess with Assetic either way, my personal preference would be just to go straight to JSqueeze.

@DeepDiver1975
Copy link
Member

let's go for JSqueeze

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants