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

[MSKINS-168] Switch Minification Engine from YUI to CLOSURE #18

Merged
merged 1 commit into from
May 27, 2020
Merged

[MSKINS-168] Switch Minification Engine from YUI to CLOSURE #18

merged 1 commit into from
May 27, 2020

Conversation

dialaya
Copy link
Contributor

@dialaya dialaya commented May 26, 2020

YUI compressor engine seems not support ES5 syntaxes.
Js compilation fails when using new javascript libraries with ES5 syntax.

Switch to CLOSURE engine to bypass the issues.

Closes #168

@dialaya dialaya marked this pull request as draft May 26, 2020 19:59
@dialaya dialaya marked this pull request as ready for review May 26, 2020 20:09
@dialaya
Copy link
Contributor Author

dialaya commented May 26, 2020

Hi @hboutemy ,
Could you please have a look at the failed check and review the pull request ?

Regards,

@michael-o
Copy link
Member

Which new files did you try to fail the build?

@dialaya
Copy link
Contributor Author

dialaya commented May 26, 2020

@michael-o What I know is that the job-14 failed during call of ExtractToDirectory.
The change I have made only specify a jscompressor engine.

Thank you.

@michael-o
Copy link
Member

I did not refer to CI, but to specific JS files you have tried.

@dialaya
Copy link
Contributor Author

dialaya commented May 26, 2020

below target JS libs are affected:

  • anchorjs
  • bootstrap 4
  • jquery 3
  • or any other new lib that uses ES5 or greater you might need to use later

Without the switch: i made changes in anchorjs lib that you can see in my previous pull request #17 that I had aborted to avoid custom maintenance.

But for jquery and bootstrap I is not a good idea to modify the syntax because of ES5 support issues.

Please see MSKINS-168 for more details.

@michael-o
Copy link
Member

Thanks, that's what I wanted to know.

@hboutemy hboutemy merged commit f966972 into apache:master May 27, 2020
@hboutemy
Copy link
Member

nice change, well explained: thank you @dialaya

@dialaya
Copy link
Contributor Author

dialaya commented May 28, 2020

Thank you

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