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

Exclude js scripts with type="module" from JS minification #6273

Open
alfonso100 opened this issue Nov 16, 2023 · 4 comments · May be fixed by #6277
Open

Exclude js scripts with type="module" from JS minification #6273

alfonso100 opened this issue Nov 16, 2023 · 4 comments · May be fixed by #6277
Labels
effort: [XS] < 1 day of estimated development time module: minify JS priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement

Comments

@alfonso100
Copy link
Contributor

alfonso100 commented Nov 16, 2023

Before submitting an issue please check that you’ve completed the following steps:
yes - Made sure you’re on the latest version
yes - Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
When the option “Minify JavaScript files” is enabled, and scripts with type="module" are moved to the min folder, is likely to cause 404 errors because a a JS file with this type is likely to have import statemets, and changing the path where the expected module is located will break these links

Per customer feedback:

  1. JavaScript with type=“module” will always have “import” statements in it that references to a files, which location or existence WP Rocket cannot detect. So if WP Rocket changes and moves the file, the paths to these files are interrupted by WP Rocket.

Example code. the files ./localization.d6c03d4a.js, ./selectors.e37ac126.js and ./vue.16a54326.js with cause 404:

import{_ as i,a as n,b as s,c as e,d as a,e as r,f as l,g as C,h as b,i as d,j as c,k as w,l as k}from"./localization.d6c03d4a.js";import{_ as t,a as g}from"./selectors.e37ac126.js";import"./vue.16a54326.js";class _{IABTCF(){return{currentTCString:"",hasWhichConsents:function(){return{}},isTCStringUpToDate:function(){return!1},signalDoNowShowUI:function(){},saveConsentAll:function(){}}}}const p=new _;i.init(window.borlabsCookieConfig.contentBlockers);n.init(window.borlabsCookieConfig.providers);s.init(window.borlabsCookieConfig.serviceGroups);e.init(window.borlabsCookieConfig.services);t.init(window.borlabsCookieConfig.settings);a.init(window.borlabsCookieConfig.globalStrings);window.borlabsCookiePrioritized===void 0&&(window.borlabsCookiePrioritized={},window.borlabsCookiePrioritized.optInCodesUnblocked={});r.optInCodesUnblocked=window.borlabsCookiePrioritized.optInCodesUnblocked;const f={Cookie:C,ContentBlocker:i,Consents:b,ConsentHistory:d,Providers:n,ScriptBlocker:c,ServiceGroups:s,Services:e,CookieLibrary:w,Settings:t,Log:g,Unblock:r,Tools:k,Adapter:p};window.BorlabsCookie=f;window.__toBorlabsPluginAssetUrl=o=>window.borlabsCookieConfig.settings.pluginUrl+"/assets/"+o;l.init();

To Reproduce
Steps to reproduce the behavior:

  1. Enqueue a JS script with the above code.
  2. create some import scripts at the same location for example: ./localization.d6c03d4a.js, ./selectors.e37ac126.js and ./vue.16a54326.js
  3. test the page without JS minification, it will be fine.
  4. Enable JS Minification
  5. See the 404 error on the imported files
  6. Exclude the origin file form JS minification
  7. see the errors fixed

Expected behavior
we should exclude type="module" from minification

Screenshots
cleanshot-2023-11-08-at-15-50-49-2x

Additional context
Slack: https://wp-media.slack.com/archives/C43T1AYMQ/p1699483184373919
ticket: https://secure.helpscout.net/conversation/2414653718/453464?folderId=2683093

Acceptance Criteria (for WP Media team use only)

@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: low Issues that can wait needs: grooming module: minify JS labels Nov 17, 2023
@CrochetFeve0251
Copy link
Contributor

@piotrbak I am just wondering how we can handle the order of the execution with that.
Imagine we have in that order:

  • moduleA
  • regularJsA
  • moduleB
  • regularJsB
    Is it possible that changing it to :
  • moduleA
  • moduleB
  • minifyA+B
    Could break something?

@BenBornschein
Copy link

If you leave JavaScripts of type module untouched, nothing should break. And there should be no JavaScript that is not of type module that depends on JavaScript of type module.

@Tabrisrp Tabrisrp self-assigned this Nov 20, 2023
@Tabrisrp Tabrisrp added effort: [XS] < 1 day of estimated development time and removed needs: grooming labels Nov 20, 2023
@Tabrisrp Tabrisrp linked a pull request Nov 20, 2023 that will close this issue
4 tasks
@Tabrisrp Tabrisrp linked a pull request Nov 20, 2023 that will close this issue
4 tasks
@piotrbak
Copy link
Contributor

Moving it back to To do, so we can take advantage of the backend. Waiting for @mostafa-hisham to move forward with https://github.com/wp-media/rucss-backend/issues/83

@Tabrisrp Tabrisrp removed their assignment Nov 27, 2023
@MathieuLamiot
Copy link
Contributor

@piotrbak The back-end part should be completed this sprint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time module: minify JS priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants