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

[5.2][bug] Codemirror duplicated assets entries #44674

Open
wants to merge 15 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Dec 28, 2024

Redo of #44671 .

Summary of Changes

(reported by Hannes)
The file media/plg_editors_codemirror/joomla.asset.json has duplicate entries.
This PR makes sure that these entries are unique

Testing Instructions

Pre-conditions

It needs development environment with PHP CLI, composer and npm on Linux or on Windows with WSL, and a git clone of either the CMS repository or your fork of that.

Open a command shell window (Terminal on Linux, CMD with WSL on Windows) and change to the root directory of your git clone.

All commands mentioned in the test are executed in that command shell window.

It is not enough just to apply this PR. It needs to have a branch with the change so it will also be available in the tag used for building the packages.

Therefore please follow strictly the testing instructions.

Test 1: Build on clean, current 5.2-dev branch

Make sure that you have checked out the 5.2-dev branch.

git checkout 5.2-dev

If you have done some work on that branch before, like e.g. running composer or npm or making an installation, make sure to clean up the current branch and revert all local changes.

git clean -d -x -f
git checkout .

Create a new tag on the local clone.

git tag -a 5.2.3 -m "test 5.2.3"

Run the build script without any parameters so the previously created new tag will be used.

php ./build/build.php

The created packages can be found in directory ./build/tmp/packages.

Save the full installation ZIP package outside of the git clone for later comparison with the result of the other tests.
E.g. if the parent folder is writable:

cp ./build/tmp/packages/Joomla_5.2.3-dev-Development-Full_Package.zip ../Joomla_5.2.3-dev-Development-Full_Package_test-1.zip

Clean up for later tests: Delete the previously created tag.

git tag -d "5.2.3"

Test 2: Build on 5.2-dev branch after composer install and npm ci

Make sure to clean up the current branch and revert all local changes.

git clean -d -x -f
git checkout .

Run composer and npm.

composer install
npm ci

Create a new tag on the local clone.

git tag -a 5.2.3 -m "test 5.2.3"

Run the build script without any parameters so the previously created new tag will be used.

php ./build/build.php

The created packages can be found in directory ./build/tmp/packages.

Save the full installation ZIP package outside of the git clone for later comparison with the result of the other tests.
E.g. if the parent folder is writable:

cp ./build/tmp/packages/Joomla_5.2.3-dev-Development-Full_Package.zip ../Joomla_5.2.3-dev-Development-Full_Package_test-2.zip

Clean up for later tests: Delete the previously created tag.

git tag -d "5.2.3"

Compare the file media/plg_editors_codemirror/joomla.asset.json from the package created in this test with the file from the package created in the previous test "Test 1".

Result: See section "Actual result BEFORE applying this Pull Request" below the testing instructions.

Test 3: Build on clean branch of this PR

Make sure to clean up the current branch and revert all local changes.

git clean -d -x -f
git checkout .

Fetch this pull request (PR) into a new local branch and check out that branch.

git fetch upstream pull/44674/head:test-pr-44674
git checkout test-pr-44674

Create a new tag on the local clone.

git tag -a 5.2.3 -m "test 5.2.3"

Run the build script without any parameters so the previously created new tag will be used.

php ./build/build.php

The created packages can be found in directory ./build/tmp/packages.

Save the full installation ZIP package outside of the git clone for later comparison with the result of the other tests.
E.g. if the parent folder is writable:

cp ./build/tmp/packages/Joomla_5.2.3-dev-Development-Full_Package.zip ../Joomla_5.2.3-dev-Development-Full_Package_test-3.zip

Clean up for later tests: Delete the previously created tag.

git tag -d "5.2.3"

Compare the file media/plg_editors_codemirror/joomla.asset.json from the package created in this test with the file from the package created in the first test 1.

Result: See section "Expected result AFTER applying this Pull Request" below the testing instructions.

Test 4: Build on branch of this PR after composer install and npm ci

Make sure to clean up the current branch and revert all local changes.

git clean -d -x -f
git checkout .

Only if you haven't done this before e.g. when executing test 3:
Fetch this pull request (PR) into a new local branch and check out that branch.

git fetch upstream pull/44674/head:test-pr-44674
git checkout test-pr-44674

Run composer and npm.

composer install
npm ci

Create a new tag on the local clone.

git tag -a 5.2.3 -m "test 5.2.3"

Run the build script without any parameters so the previously created new tag will be used.

php ./build/build.php

The created packages can be found in directory ./build/tmp/packages.

Save the full installation ZIP package outside of the git clone for later comparison with the result of the other tests.
E.g. if the parent folder is writable:

cp ./build/tmp/packages/Joomla_5.2.3-dev-Development-Full_Package.zip ../Joomla_5.2.3-dev-Development-Full_Package_test-4.zip

Clean up for later tests: Delete the previously created tag.

git tag -d "5.2.3"

Compare the file media/plg_editors_codemirror/joomla.asset.json from the package created in this test with the file from the package created in the second test "Text 2".

Compare the file media/plg_editors_codemirror/joomla.asset.json from the package created in this test with the file from the package created in the previous test "Text 3".

Result: See section "Expected result AFTER applying this Pull Request" below the testing instructions.

Actual result BEFORE applying this Pull Request

When creating a new tag and building packages for that tag after having executed composer installand npm ci (or npm ior npm install), file media/plg_editors_codemirror/joomla.asset.json contains duplicate script assets and dependencies.
2024-12-29_1

2024-12-29_2

2024-12-29_3

On a clean branch without having executed composer installand npm ci this does not happen.

Besides this, the order of assets in the mentioned file or any other joomla.asset.json file might be different to the result from a build on a clean branch.

Expected result AFTER applying this Pull Request

Creating a new tag and building packages for that tag on a clean branch, i.e. no previous composer or npm run, works with this PR applied as well as without.

When having run composer or npm run before that, the issue does not happen anymore with this PR applied.

The order of assets in the mentioned file or any other joomla.asset.json file might be different to the result from a build on a clean branch.
This could maybe be fixed with another PR.

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

@richard67

@richard67
Copy link
Member

richard67 commented Dec 28, 2024

@dgrammatiko For reproducing the issue it is not enough to follow the current testing instructions. In fact it's a bit more complicated. It needs to run composer install and npm cion the clean, local branch, then create a new git tag on that local branch and then run php ./build/build.php without any parameters so the latest local tag is used.

I will see if I can provide detailed testing instructions, but it might be tomorrow.

@dgrammatiko
Copy link
Contributor Author

Could you do me favor? When testing this could you switch to 5.3, add a "type": "module", line in the package.json and then test if that happens there? I'm guessing it's the module resolution algo from NodeJS

@richard67
Copy link
Member

richard67 commented Dec 28, 2024

Could you do me favor? When testing this could you switch to 5.3, add a "type": "module", line in the package.json and then test if that happens there? I'm guessing it's the module resolution algo from NodeJS

@dgrammatiko You mean with the changes of this PR here included? Or you mean on a clean 5.3-dev? And where exactly should I add the "type": "module", line?

@dgrammatiko
Copy link
Contributor Author

Or you mean on a clean 5.3-dev? And where exactly should I add the "type": "module", line?

on a clean 5.3 and the "type": "module", could be inserted as the first entry at the top of the package.json. fwiw the resolution algo is different for cjs and esm, so maybe we're lucky and 5.3 already solves this with a simple line in the package.json...

@richard67
Copy link
Member

richard67 commented Dec 28, 2024

Hmm, I've just tested this PR, and now the gz files are not created at all. I will continue to test tomorrow.

Update: False alarm. I've checked the wrong media folder when comparing.

@richard67
Copy link
Member

@dgrammatiko I've allowed myself to provide testing instructions so it's clear how the issue can be reproduced.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 2152bf2


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

@Fedik
Copy link
Member

Fedik commented Dec 30, 2024

Interesting, so it is actually /nodel_modules gets duplicated while doing all that.
Thanks for investigation.

@dgrammatiko please just update

module.exports.getPackagesUnderScope = (scope) => {

To use Set():

module.exports.getPackagesUnderScope = (scope) => {
  const cmModules = new Set();

  // Get the scope roots
  const roots = [];
  module.paths.forEach((path) => {
    const fullPath = `${path}/${scope}`;
    if (existsSync(fullPath)) {
      roots.push(fullPath);
    }
  });

  // List of modules
  roots.forEach((rootPath) => {
    readdirSync(rootPath).forEach((subModule) => {
      cmModules.add(`${scope}/${subModule}`);
    });
  });

  return [...cmModules];
};

Thanks!

@richard67
Copy link
Member

I have tested this item ✅ successfully on b057e7c


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

1 similar comment
@Fedik
Copy link
Member

Fedik commented Dec 31, 2024

I have tested this item ✅ successfully on b057e7c


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

@Fedik
Copy link
Member

Fedik commented Dec 31, 2024

r2c


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 31, 2024
@Fedik Fedik added the bug label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PR-5.2-dev RTC This Pull Request is Ready To Commit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants