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

[4.2] NPM build tools cleanup #38225

Closed
wants to merge 10 commits into from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Jul 4, 2022

Pull Request for Issue # .

Summary of Changes

  • remove some I/O operations on both the JS and SCSS minification (eg use the code output directly)
  • Ran npm lint:js -- --fix and committed the changes
    • Removed the IIFE from build/media_source/com_content/js/articles-status.es6.js as the file is correctly loaded either as module or when in nomodule it's using the defer attribute, thus adding the listener for DOMContentLoaded is pure overhead. At that point the DOM is already there fully parsed. This needs to be applied ON EVERY SCRIPT

Testing Instructions

On a fresh installation of Joomla (ie git clone) do the composer install and then the npm install. Copy the media folder somewhere in your hardisk. (check the time needed)
Now clone this PR and do both the composer and npm installations. Once done do a folder compare of the current media folder against the folder that you saved in the previous step. You should have the same files apart from the build/media_source/com_content/js/articles-status.es6.js which will have different content (expected).

Compare the times and do a brief check that both front end and back still work as expected

Actual result BEFORE applying this Pull Request

Screenshot 2022-07-04 at 18 36 53

Expected result AFTER applying this Pull Request

Screenshot 2022-07-04 at 18 35 50

Documentation Changes Required

NO

@HLeithner have somebody merge this so I could do the changes in the v5.0. Thanks

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels Jul 4, 2022
@brianteeman
Copy link
Contributor

the change to image.vue is not correct. The span element is not self closing. Both parts are mandatory.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Jul 4, 2022

the change to image.vue is not correct. The span element is not self closing. Both parts are mandatory.

On your repo (4.2) try npm run lint:js -- --fix, you will get the same. The HTML inside the .vue files is totally fine with this change and actually it's the recommendation, (it's the v- part that makes the difference)

btw here's the eslint rule and the explanation: https://eslint.vuejs.org/rules/html-self-closing.html

@brianteeman
Copy link
Contributor

Before

image

After

image

@dgrammatiko dgrammatiko deleted the 4.2-dev-buildtools branch July 12, 2022 22:04
@dgrammatiko dgrammatiko restored the 4.2-dev-buildtools branch August 29, 2022 14:21
@dgrammatiko dgrammatiko reopened this Aug 29, 2022
@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev August 29, 2022 14:26
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.3-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants