Skip to content

[4.4] Move to ESM#43779

Closed
dgrammatiko wants to merge 15 commits intojoomla:4.4-devfrom
dgrammatiko:mjs
Closed

[4.4] Move to ESM#43779
dgrammatiko wants to merge 15 commits intojoomla:4.4-devfrom
dgrammatiko:mjs

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jul 12, 2024

Pull Request for Issue # .

Summary of Changes

  • Move tools and source JS to ESM
  • ES6 files have now the extension .mjs (used to be .es6.js)
  • Web Components also have .mjs extension (used to be .w-c.es6.js)
  • The build/media_source/system/js/joomla-core-loader.mjs file now includes the required css and the related build/media_source/system/scss/joomla-core-loader.scss is deleted

Testing Instructions

You need Git and NPM to test this

Pull the 4.4-dev branch and run:

  • npm install
  • npm run gzip
  • npm run versioning
  • Store the media folder somewhere outside of the joomla-cms folder

Pull this PR gh pr checkout 43779 and run:

  • npm install
  • npm run gzip
  • npm run versioning
  • Store the media folder somewhere outside of the joomla-cms folder

Compare the two folders. The only difference should be the missing files: media/system/css/joomla-core-loader.css, media/system/css/joomla-core-loader.min.css, media/system/css/joomla-core-loader.min.css.gz

Check that basic backend functionality

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@Fedik @richard67 could you please test this one (pretty hard for the average tester)?

@LadySolveig @bembelimen please DO NOT upmerge this, I will do a PR on the 5.1 instead

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.4-dev labels Jul 12, 2024
@dgrammatiko dgrammatiko marked this pull request as ready for review July 12, 2024 18:30
Co-authored-by: Brian Teeman <brian@teeman.net>
@Fedik
Copy link
Member

Fedik commented Jul 13, 2024

Thanks for the work.

I do not like renaming under /media_source. We do not have actual modules there, only regular scripts. I would call .mjs only for scripts that actualy use ESM (have export or import in it) There only few of then (even web components not really utilise it). Personaly I would not bother with it.
But I don't mind, if release managers decide to accept it.

For the build script it looks much better there. Because there is actual ESM now.

@dgrammatiko
Copy link
Contributor Author

For the build script it looks much better there. Because there is actual ESM now.

@Fedik it's just a convention, but now we have way less options, which, normally, should be easier for every contributor

@Fedik
Copy link
Member

Fedik commented Jul 13, 2024

should be easier for every contributor

yeah

@Fedik
Copy link
Member

Fedik commented Jul 17, 2024

I have tested this item ✅ successfully on 6b3316b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43779.

@laoneo
Copy link
Member

laoneo commented Jul 24, 2024

Thanks for your hard work on this. Unfortunately 4.4 is in maintenance mode, where we merge mainly bug fixes. To not risk to have the 4.4-dev unstable and prevent side effects in the ecosystem, I'm closing this one for now as the move to ESM should be done in an active development branch, which is currently 5.2. Thanks for understanding.

@laoneo laoneo closed this Jul 24, 2024
@dgrammatiko
Copy link
Contributor Author

@laoneo the only reason I backported the changes was to make it easier for devs when they'll have to push a change from 4.4 to 5/6. Anyways np

@dgrammatiko dgrammatiko deleted the mjs branch July 24, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.4-dev

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants