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 js-beautify and html-minifier to cli #2059

Merged
merged 1 commit into from Oct 21, 2020

Conversation

TrySound
Copy link
Contributor

@TrySound TrySound commented Oct 9, 2020

Ref #1947

  • minify outlook conditionals unconditionally like merging
  • moved js-beautify and html-minifier to cli
  • deprecated "minify" and "beautify" options in mjml-core

Ref mjmlio#1947

- minify outlook conditionals unconditionally like merging
- moved js-beautify and html-minifier to cli
- deprecated "minify" and "beautify" options in mjml-core
@TrySound
Copy link
Contributor Author

TrySound commented Oct 9, 2020

сс @kmcb777 @iRyusa @DRoet

@iRyusa
Copy link
Member

iRyusa commented Oct 10, 2020

Looks good to me cc @ngarnier 5.X branch will remove those from core maybe we should discuss if we have more breaking changes to push in it ?

@ngarnier
Copy link
Member

Ok yeah @iRyusa let's chat this week

@iRyusa
Copy link
Member

iRyusa commented Oct 21, 2020

We've just discussed about this @TrySound and we're ok to remove both of them in MJML5. However we still want to provide somekind of "easy" way to get them back. I think it could be done with .mjmlconfig where you can list postRender hooks and register functions + arguments ?

{
  postRender: { "packageName": [{ options}, anotherArgs, anotherArgs] }
}

I'm not sure about the format tho, and if dynamic require would be an issue with browser build (webpack/rollup/...).

Can we discuss about this together so we can ease the transition in MJML5 with proper doc on that ?

@TrySound
Copy link
Contributor Author

I think we can provide custom plugins with hooks which run any minification or other tool. I can implement it in separate PR and default to it in cli.

@iRyusa
Copy link
Member

iRyusa commented Oct 21, 2020

That would be so cool 👍 I'm merging this one as it's fine for MJML 4.8 then

@iRyusa iRyusa merged commit c987912 into mjmlio:master Oct 21, 2020
@TrySound
Copy link
Contributor Author

Will do this in a couple of weeks after vacation.

@IanEdington
Copy link
Contributor

@TrySound It seems like this will this break minification in the gulp tool. Is that true?

@iRyusa
Copy link
Member

iRyusa commented Oct 21, 2020

Nope it wont the option will still be handled in core but display a warning during the transition. We'll just add those into gulp-mjml dependencies that's not that hard

@TrySound
Copy link
Contributor Author

Or piped gulp plugins can be used instead to not bloat gulp-mjml and make it single purpose.

@TrySound TrySound deleted the cli-dependencies branch October 21, 2020 20:26
@IanEdington
Copy link
Contributor

@iRyusa Thanks for pointing that out 👍

@TrySound I like the idea of simplifying MJML by removing these dependencies. That approach seems like a good one for a v5 change.

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.

None yet

4 participants