adobe / asset-share-commons

A modern, open-source asset share reference implementation built on Adobe Experience Manager (AEM)
https://opensource.adobe.com/asset-share-commons/
Apache License 2.0
88 stars 107 forks source link

Minification on clientlib-dependencies throwing JS errors #856

Closed kenwoodward closed 1 year ago

kenwoodward commented 1 year ago

Describe the bug The recent change to the jsProcessor on client-dependencies is causing jQuery to break. The vendor JS is being minified when previewing a page as published (wcmmode=disabled). The JS is minified be default. The jsProcessor needs to be set to min:none; default:none for already minified files.

Environment

To Reproduce Steps to reproduce the behavior:

  1. Go to default light or dark page or a page using the Search Template.
  2. View page as published (wcmmode=disabled)
  3. Click on Asset Filters (they will not work)
  4. View JS Console in browser
  5. 3 JS errors will be present (Uncaught TypeError: Cannot read properties of undefined (reading 'html5'), Uncaught ReferenceError: jQuery is not defined (2x)).

Expected behavior Search result filters should be interactive

Screenshots Screen Shot 2022-11-21 at 4 03 58 PM

Additional context See the following commit: https://github.com/adobe/asset-share-commons/commit/377f28030d36b3336f68de6adb57e5ebb982cfde#diff-3d9603a79a2e50ac5a438eec3844e6f6cd0a87484f2e5bb69c92ed18f1326817

davidjgonzalez commented 1 year ago

@kwin was this change working for you?

kenwoodward commented 1 year ago

@davidjgonzalez I tested this on a customer implementation and setup a fresh local implementation and saw the same behavior. Was hoping @kwin could verify.

kwin commented 1 year ago

@kenwoodward Can you come up with a fix? I won’t be able to dig into this this week but even double minification shouldn’t be a problem. Do you see errors in the log when the minified clientlib is built?

kenwoodward commented 1 year ago

@kwin Setting the jsProcessor back to "min:none; default:none" on that client library fixes the issue. I will dig a little deeper today and see if I can find what exactly is occurring during minification before submitting any PRs. I've tried adding each vendor clientlib individually and at first glance it seems that modernizr and jquery are not playing nice together when minimized a second time.

kenwoodward commented 1 year ago

@kwin I am getting 1 warning when compiling that clientlib:

com.google.javascript.jscomp /apps/asset-share-commons/clientlibs/clientlib-dependencies.js:340: WARNING - unreachable code...

That warning is followed by a printout of the clientlib and ending with:

e&&C.jQuery===k&&(C.jQuery=Qt),k},e||(C.jQuery=C.$=k),k}); ^^^^^^^^^

Not sure if that is relevant to this issue or not.

davidjgonzalez commented 1 year ago

@kenwoodward @kwin

/apps/asset-share-commons/clientlibs/clientlib-dependencies

Setting jsProcessor to min:gcc;languageIn=ECMASCRIPT_2015;languageOut=ECMASCRIPT_2019, default:none seems to work.

kenwoodward commented 1 year ago

@davidjgonzalez @kwin

Certainly, that change will work.

The problem seems to be with the version of Modernizr and ES5 vs ES6.

Here is what i did to confirm the problem.

  1. Changed the jsProcessor to min:gcc;languageOut=ECMASCRIPT_2015 and it fixed the JS issues. This essentially just set "languageIn" to ECMASCRIPT5.
  2. I reset the jsProcessor back to its ASC default - min:gcc;languageIn=ECMASCRIPT_2015;anguageOut=ECMASCRIPT_2015
  3. I reviewed the order the JS was being added to the clientlib and Modernizr was the first vendor code added (it follows the Granite code).
  4. I removed Modernizr and it fixed the issue
  5. I added Modernizr back and updated it to version 3.6.0 and it fixed the issue

The currently used version of Modernizr in ASC is 2.8.3.

I see 3 potential solutions here:

  1. Change the jsProcessor value to min:gcc;languageOut=ECMASCRIPT_2015 (or to David's suggestion) for clientlib-dependencies
  2. Update Modernizr to 3.6.0
  3. Remove Modernizr - an argument could be made to how relevant this is now.
davidjgonzalez commented 1 year ago

@kenwoodward I like options 3 and 2 .. TBH im not sure what the impact of removing modernizr would be at this point. Might be easier to upgrade to 3.6.0?

Or we could do a quick smoketest with removing modernizr. (Less is more IMO, so if we can get rid of something - im all about that)

kenwoodward commented 1 year ago

@davidjgonzalez the quick fix would be to update Modernizr.

I agree that less is more and the better fix would be to smoke test the removal of Modernizr (and maybe even FormData Polyfill).

davidjgonzalez commented 1 year ago

Do you have a link to modernizr 3.6.0 download you tried it with?

Idk why all I'm seeing is the "build you own" ...

kenwoodward commented 1 year ago

I grabbed this from the top of the current ASC version. It was a custom build before. This will give you the 3.6.0 version

https://modernizr.com/download/?applicationcache-audio-backgroundsize-borderimage-borderradius-boxshadow-canvas-canvastext-cssanimations-csscolumns-cssgradients-cssreflections-csstransforms-csstransforms3d-csstransitions-flexbox-fontface-generatedcontent-geolocation-hashchange-history-hsla-indexeddb-inlinesvg-input-inputtypes-localstorage-multiplebgs-opacity-postmessage-rgba-sessionstorage-smil-svg-svgclippaths-textshadow-video-webgl-websockets-websqldatabase-webworkers-addtest-domprefixes-hasevent-mq-prefixed-prefixes-setclasses-shiv-testallprops-testprop-teststyles

kenwoodward commented 1 year ago

You beat me to it. :)

davidjgonzalez commented 1 year ago

tested out updating to that v3.6.0 and looks good. Cutting v2.4.6 with it now!

davidjgonzalez commented 1 year ago

Thanks for helping solve this one @kenwoodward! I also made an issue to update/remove some of the legacy JS/CSS stuff in the project aswell.

davidjgonzalez commented 1 year ago

@kenwoodward done! 2.4.6 has been released, usually takes a few mins to sync across maven central.