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

dgrammatiko · 2024-12-27T16:02:01Z

Pull Request for Issue # .

Summary of Changes

(reported by Hannes)
The file media/plg_editors_codemirror/joomla.asset.json has duplicate entries.
This PR adds a simple check so the entries are unique

Testing Instructions

Remove the media folder
Run npm ci
Check that the generated file media/plg_editors_codemirror/joomla.asset.json doesn't have any duplicated entries

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

@Hackwar here you go

brianteeman · 2024-12-27T16:18:07Z

What are the duplicate entries?

dgrammatiko · 2024-12-27T16:28:08Z

Any of the scripts that are registered as importmap, ie @codemirror/lang-css

brianteeman · 2024-12-27T17:06:57Z

I dont see any duplicates on my local build but I do see it on a live site - weird

dgrammatiko · 2024-12-27T17:26:57Z

I dont see any duplicates on my local build but I do see it on a live site - weird

I think it happens within the build process, anyways the code here should be sufficient

richard67 · 2024-12-27T17:38:43Z

@brianteeman It seems to happen only in some cases. I was not able to reproduce it locally either. But if you check any of the 5.x installation or update packages, e.g. 5.0.0, …, 5.2.2, you will see that in almost all of them there are the mentioned duplicate entries, with only a few exceptions, e.g. 5.2.0.

brianteeman · 2024-12-27T19:16:43Z

I cant test it as I cannot replicate the bug therefore I cannot test if this fixes it

Hackwar · 2024-12-27T21:48:13Z

The problem occurs when running build/build.php on a Debian system to create a release package. It does NOT happen when you simply run npm i. The fact that the RMs for 5.0, 5.1 and 5.2 with differing build systems all had this problem, should be enough to consider the bug as confirmed. I will test this tomorrow and I hope that there will be another person to test this, so that we can add this to 5.2.4.

richard67 · 2024-12-27T21:51:04Z

Well for me it does not happen when using build.php, possibly because I use Ubuntu and not Debian.

Fedik · 2024-12-27T22:06:38Z

I think we should really check what is going on with build process, because it may also have other hidden impact or will have in future.

The core issue that media/plg_editors_codemirror/joomla.asset.json (and probably whole build files) did not removed betwen runs, in some reason.
I can guess it is 1 run for full packge and 1 run for upgrade packge? (I did not tried to reproduce, just guessing)

Current PR is a workaround, and it is fine.
But the bug will stay until we will not figure out what is really wrong.

richard67 · 2024-12-27T22:13:46Z

I can guess it is 1 run for full packge and 1 run for upgrade packge? (I did not tried to reproduce, just guessing)

@Fedik No, the build runs only once. After full packages are packed, certain files are removed which do not belong to update packages, and then the update packages are packed, all from the same result of one build. Composer runs only once, and npm also only once.

dgrammatiko · 2024-12-28T08:44:58Z

The core issue that media/plg_editors_codemirror/joomla.asset.json (and probably whole build files) did not removed betwen runs, in some reason.

just add an rm -rf media in build.php right before the copy everything line.
Anyways this pr just adds a simple check if an asset is already present in the json and if so skip it, no harm to have it…

richard67 · 2024-12-28T09:29:34Z

just add an rm -rf media in build.php right before the copy everything line.

Unfortunately I think it might not be that easy.

The build.php creates a temporary folder with a name based on the current time, so that would be a new one with every run, and to play safe it even does an rm -rf before creating it.

Then the build.php does not copy stuff from the current branch in that folder, it does a git fetch into to that folder then changes directory to that folder and runs composer install and npm ci in that folder.

So from my understanding it can not happen that the media folder exists in that folder when it runs.

So right now I have no explanation for how the issue can happen, and I can’t reproduce it here with running the build.php, even not when running it with the default remote (latest tag) like release managers do when building releases.

Anyways this pr just adds a simple check if an asset is already present in the json and if so skip it, no harm to have it…

I agree.

richard67 · 2024-12-28T09:37:27Z

P.S.: The only way which I can imagine how it could happen is that one of the other js scripts which runs after the build-codemirror.es6.js touches these assets again.

If this is the case, this PR will not help.

Let’s wait for Hannes’ test result as he was able to reliably reproduce the issue.

richard67 · 2024-12-28T09:43:49Z

P.P.S. Or there is something wrong with some asynchronous promise or whatever stuff so the compileCodemirror function is run 2 times in some case.

dgrammatiko · 2024-12-28T10:07:01Z

Turns out that the media folder shouldn't be the root cause here. My theory is that the resolution of modules is returning, in some cases, both the cjs and the esm entries, thus the duplicates. ie:

{
2  "name": "@codemirror/lang-php",
3  "version": "6.0.1",
4  "description": "PHP language support for the CodeMirror code editor",
5  "scripts": {
6    "test": "cm-runtests",
7    "prepare": "cm-buildhelper src/php.ts"
8  },
9  "keywords": [
10    "editor",
11    "code"
12  ],
13  "author": {
14    "name": "Marijn Haverbeke",
15    "email": "[email protected]",
16    "url": "http://marijnhaverbeke.nl"
17  },
18  "type": "module",
19  "main": "dist/index.cjs",
20  "exports": {
21    "import": "./dist/index.js",
22    "require": "./dist/index.cjs"
23  }
}

The exports has 2 entries so if it's not specifically checking for the import then each package has 2 versions thus the duplicates in the assets.json.

All I'm saying is that the root cause is somewhere in build/build-modules-js/init/common/resolve-package.es6.js

Should/could it patched in that point? The answer is yes but I wouldn't do that before moving the code to esm, currently it's still cjs. Maybe for 5.4...

richard67 · 2024-12-28T10:57:23Z

Ha, I was (partly) wrong, and I've found a way how to reproduce the issue, and it is the media folder being already present which causes the problem!!!

The build.php does not do a git fetch, it uses the git archive command.

When a release is made, a new tag is created on the local clone locally.

The build script then uses that tag for the git archive command.

Because that tag does not exist on the remote, the command uses the local tag.

If you now have run composer install and npm ci on that local branch before creating the local tag, which is not necessary for building a release and which should not be done, but it seems that was the case, then the git archive indeed seems to copy the branch.

Now you run php ./build/build.php without any parameter. This then executes git archive tags/<yourTag> | tar -x -C <fullPath>, with <yourTag> being the previously created, newest tag on the local branch, and <fullPath> being the full path to the new temporary folder.

This then seems to just copy the complete stuff.

There are 2 ways to fix it in build.php:

Release managers have to clean up the local branch with git clean -d -x -f before creating the tag and using build.php.
That would be something to fix in the internal documentation.
Let build.php remove the media folder in the temporary folder after the git archive call.
That could be done alternatively or in addition to the previous way.

~~Update: Now I could reproduce it also without having run composer and npm before creating the tag. So I might be wrong. But I will try if the 2nd fix helps.~~ False alarm, looked at the wrong file. The above description is right.

brianteeman · 2024-12-28T11:12:12Z

In other words this pr is not needed and the problem was in the way the build process was done

richard67 · 2024-12-28T11:15:50Z

I will prepare a PR for build.php.

@dgrammatiko Thanks a lot for your efforts and sorry for the noise.

richard67 · 2024-12-28T11:42:03Z

Hmm, seems I was again wrong. In the scenario in which I can reproduce the issue, the media folder does not exist after the git archive call, so my suggested 2nd fix doesn't help. I will continue to investigate.

dgrammatiko · 2024-12-28T12:00:26Z

@richard67 the media folder shouldn’t be the root cause because the assets.json is always recreated from the build source and the extra assets are just appended. The file in the media folder is always overwritten…

richard67 · 2024-12-28T12:43:16Z

Hmm, I've just checked with a branch which contains the fix of this PR so that the modified js is used during the build process, and that does also not fix the issue.

So the only thing I can imagine is that the media/plg_editors_codemirror/joomla.asset.json file is touched again by another js function running after the npm install --unsafe-perm call, e.g. the npm run cssversioning or the npm run gzip or the npm run versioning, or it is the build/build.js --prepare doing that already.

richard67 · 2024-12-28T13:00:31Z

Weird, the issue even is reproducable when I exit the build.php directly after the npm install --unsafe-perm call, so the npm run cssversioning or npm run gzip or npm run versioning can't be the culprit.

@dgrammatiko Do you know why in the build/build.js the if (cliOptions.prepare) { comes at the very end?

dgrammatiko · 2024-12-28T13:41:23Z

Do you know why in the build/build.js the if (cliOptions.prepare) { comes at the very end?

The order should be irrelavent as any of those blocks execute only when the condition is truthy.

@richard67 could you remove

joomla-cms/build/build-modules-js/init/minify-vendor.es6.js

Line 11 in e74c6ac

'media/vendor/codemirror',

and check if there's any improvement?

If that won't do anything (shouldn't) then can you try rearranging the build.js to

if (cliOptions.prepare) {
  const bench = new Timer('Build');
  allowedVersion();
  recreateMediaFolder(options)
    .then(() => cleanVendors())
    .then(() => localisePackages(options))
    .then(() => patchPackages(options))
    .then(() => minifyVendor())
    .then(() => compileCodemirror())
    .then(() => createErrorPages(options))
    .then(() => stylesheets(options, Program.args[0]))
    .then(() => scripts(options, Program.args[0]))
    .then(() => mediaManager())
    .then(() => bootstrapJs())
    .then(() => bench.stop('Build'))
    .catch((err) => handleError(err, -1));
}

(basically the code mirror code runs before the tinymce plugin highlighter)

richard67 · 2024-12-28T13:56:43Z

@dgrammatiko I've tried something else, and that seems to help. However I'm not sure if it's the right solution as it might clean up the media/vendor/codemirrorfolder multiple times.

I have added the following code to the module.exports.recreateMediaFolder method in the build/build-modules-js/init/recreate-media.es6.js file:

    if (vendor.startsWith('@codemirror')) {
      return 'vendor/codemirror';
    }

So later that folder will be cleaned up.

The complete code for that:

    if (vendor === 'choices.js') {
      return 'vendor/choicesjs';
    }
    if (vendor.startsWith('@codemirror')) {
      return 'vendor/codemirror';
    }
    if (vendor === '@fortawesome/fontawesome-free') {
      return 'vendor/fontawesome-free';
    }
    if (vendor === '@claviska/jquery-minicolors') {
      return 'vendor/minicolors';
    }
    if (vendor === '@webcomponents/webcomponentsjs') {
      return 'vendor/webcomponentsjs';
    }
    if (vendor === 'joomla-ui-custom-elements') {
      return 'vendor/joomla-custom-elements';
    }
    return `vendor/${vendor}`;
  });

  // Clean up existing folders
  [...options.settings.cleanUpFolders, ...installedVendors].forEach((folder) => {
    const folderPath = join(`${RootPath}/media`, folder);
    if (existsSync(folderPath)) {
      emptyDirSync(folderPath);
    }
  });

@dgrammatiko Could that be the way?

richard67 · 2024-12-28T14:01:44Z

... or would it be easier just to add vendor/codemirror to the options.settings.cleanUpFolders? But where does this options.settings.cleanUpFolders come from?

dgrammatiko · 2024-12-28T14:16:42Z

But where does this options.settings.cleanUpFolders come from?

joomla-cms/build/build-modules-js/init/recreate-media.es6.js

Line 25 in e74c6ac

options.settings.cleanUpFolders = [...extensions, ...knownDirs];

richard67 · 2024-12-28T16:13:58Z

@richard67 could you remove

joomla-cms/build/build-modules-js/init/minify-vendor.es6.js

Line 11 in e74c6ac

'media/vendor/codemirror',

and check if there's any improvement?

@dgrammatiko As you have expected, that does not help.

If that won't do anything (shouldn't) then can you try rearranging the build.js to

if (cliOptions.prepare) {
  const bench = new Timer('Build');
  allowedVersion();
  recreateMediaFolder(options)
    .then(() => cleanVendors())
    .then(() => localisePackages(options))
    .then(() => patchPackages(options))
    .then(() => minifyVendor())
    .then(() => compileCodemirror())
    .then(() => createErrorPages(options))
    .then(() => stylesheets(options, Program.args[0]))
    .then(() => scripts(options, Program.args[0]))
    .then(() => mediaManager())
    .then(() => bootstrapJs())
    .then(() => bench.stop('Build'))
    .catch((err) => handleError(err, -1));
}

(basically the code mirror code runs before the tinymce plugin highlighter)

That did also not help.

And my previously mentioned fix in the "recreate-media.es6.js" file seems not to help either - when I had tested again it did not help. Maybe I made a mistake with the first test or I don't know.

dgrammatiko · 2024-12-28T16:22:28Z

Ok, one more try:
change in this file of this PR the line:

modules.forEach((module) => {

to

const uniquePackages = [...new Set(modules.map(item => item.package))];
uniquePackages.forEach((module) => {

richard67 · 2024-12-28T16:25:28Z

@dgrammatiko But it doesn't need the modification of this PR in addition, right? You only meant to do that in the same file, but it can be the unmodified one without this PR, right?

dgrammatiko · 2024-12-28T16:26:38Z

Yeah the conditional could be omitted

dgrammatiko · 2024-12-28T16:32:46Z

Meh, the code above won't work,

const uniquePackages = modules.filter((a, i) => modules.findIndex((s) => a.package === s.package) === i);
uniquePackages.forEach((module) => {

richard67 · 2024-12-28T16:49:45Z

@dgrammatiko It fixes the duplicate script assets in the file, but not the duplicates in the dependencies of the codemirror plugin script. Those are still there:

    {
      "name": "codemirror",
      "type": "script",
      "uri": "plg_editors_codemirror/codemirror.min.js",
      "importmap": true,
      "dependencies": [
        "@codemirror/autocomplete",
        "@codemirror/commands",
        "@codemirror/lang-css",
        "@codemirror/lang-html",
        "@codemirror/lang-javascript",
        "@codemirror/lang-json",
        "@codemirror/lang-markdown",
        "@codemirror/lang-php",
        "@codemirror/lang-xml",
        "@codemirror/language",
        "@codemirror/lint",
        "@codemirror/search",
        "@codemirror/state",
        "@codemirror/theme-one-dark",
        "@codemirror/view",
        "@codemirror/autocomplete",
        "@codemirror/commands",
        "@codemirror/lang-css",
        "@codemirror/lang-html",
        "@codemirror/lang-javascript",
        "@codemirror/lang-json",
        "@codemirror/lang-markdown",
        "@codemirror/lang-php",
        "@codemirror/lang-xml",
        "@codemirror/language",
        "@codemirror/lint",
        "@codemirror/search",
        "@codemirror/state",
        "@codemirror/theme-one-dark",
        "@codemirror/view",
        "@lezer/common",
        "@lezer/css",
        "@lezer/highlight",
        "@lezer/html",
        "@lezer/javascript",
        "@lezer/json",
        "@lezer/lr",
        "@lezer/markdown",
        "@lezer/php",
        "@lezer/xml",
        "@lezer/common",
        "@lezer/css",
        "@lezer/highlight",
        "@lezer/html",
        "@lezer/javascript",
        "@lezer/json",
        "@lezer/lr",
        "@lezer/markdown",
        "@lezer/php",
        "@lezer/xml"
      ],
      "version": "32671a"
    },

richard67 · 2024-12-28T16:51:33Z

So it seems we already (or also) have duplicates in the externalModules parameter.

richard67 · 2024-12-28T17:21:02Z

@dgrammatiko When I - in addition to your suggested fix - change also the line

asset.dependencies = externalModules;

to

asset.dependencies = externalModules.filter((a, i) => externalModules.findIndex((s) => a === s) === i);

The issue is solved.

However I am not sure if this is the right fix. Maybe it should be done for the cmModules and lModules in lines 90 and 91 below.

But it points us at least to the right direction.

dgrammatiko · 2024-12-28T18:49:07Z

@richard67 can you check #44674 ?

When I get some time I will try to debug this further

richard67 · 2024-12-28T19:00:43Z

@dgrammatiko I will check later tonight or tomorrow morning, but from a first look I think #44674 will fix only the duplicate dependencies of the codemirror script asset but not the duplicate assets, as in line 108 below, the unmodified cmModules is still used:

cmModules.forEach((module) => {

But I haven't tested yet. As said, that might take some time.

dgrammatiko · 2024-12-28T19:04:30Z

Good catch

Update build-codemirror.es6.js

a961bf7

joomla-cms-bot added the PR-5.2-dev label Dec 27, 2024

dgrammatiko added 2 commits December 27, 2024 18:31

Update build-codemirror.es6.js

c4d8434

Update build-codemirror.es6.js

96a8f47

richard67 added the bug label Dec 27, 2024

dgrammatiko closed this Dec 28, 2024

dgrammatiko deleted the patch-1 branch December 28, 2024 11:26

dgrammatiko mentioned this pull request Dec 28, 2024

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

Open

4 tasks

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

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

Conversation

dgrammatiko commented Dec 27, 2024

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Link to documentations

brianteeman commented Dec 27, 2024

dgrammatiko commented Dec 27, 2024

brianteeman commented Dec 27, 2024

dgrammatiko commented Dec 27, 2024

richard67 commented Dec 27, 2024

brianteeman commented Dec 27, 2024

Hackwar commented Dec 27, 2024

richard67 commented Dec 27, 2024

Fedik commented Dec 27, 2024 • edited Loading

richard67 commented Dec 27, 2024 • edited Loading

dgrammatiko commented Dec 28, 2024

richard67 commented Dec 28, 2024

richard67 commented Dec 28, 2024 • edited Loading

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024 • edited Loading

richard67 commented Dec 28, 2024 • edited Loading

brianteeman commented Dec 28, 2024

richard67 commented Dec 28, 2024

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024

richard67 commented Dec 28, 2024

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024 • edited Loading

richard67 commented Dec 28, 2024

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024

richard67 commented Dec 28, 2024

richard67 commented Dec 28, 2024 • edited Loading

richard67 commented Dec 28, 2024

dgrammatiko commented Dec 28, 2024

richard67 commented Dec 28, 2024 • edited Loading

dgrammatiko commented Dec 28, 2024

Fedik commented Dec 27, 2024 •

edited

Loading

richard67 commented Dec 27, 2024 •

edited

Loading

richard67 commented Dec 28, 2024 •

edited

Loading

dgrammatiko commented Dec 28, 2024 •

edited

Loading

richard67 commented Dec 28, 2024 •

edited

Loading

dgrammatiko commented Dec 28, 2024 •

edited

Loading

richard67 commented Dec 28, 2024 •

edited

Loading

richard67 commented Dec 28, 2024 •

edited

Loading