The module duplication check is insufficient

[Reinmar]

There's a case which the current check won't catch.

Background: the current check relies on a single ckeditor5-utils/src/version module which registers itself in the global scope and, if there's a duplicated version of this package (happens when adding plugins to an existing build or having completely missmatched versions of dependencies leading to duplicated packages in node_modules). Thanks to that, it's able to tell that it's been loaded second time and we're able to warn the developer about this situation.

However, it's not going to catch situations with conflicting versions of dependencies, but in a way which doesn't lead to a duplication of the ckeditor5-utils package. E.g.

ckeditor5-foo: 1.0.0
   requires:
     ckeditor5-engine: ^12.0.0
        requires:
           ckeditor5-utils: ^11.0.0
     ckeditor5-utils: ^11.0.0

ckeditor5-bar: 1.0.0
  requires:
     ckeditor5-engine: ^13.0.0
        requires:
           ckeditor5-utils: ^11.0.1

If your setup requires ckeditor5-foo and ckeditor5-bar npm/yarn will (I think) install:

• ckeditor5-foo@1.0.0
• ckeditor5-bar@1.0.0
• ckeditor5-engine@12.0.0
• ckeditor5-engine@13.0.0
• ckeditor5-utils@11.0.1

So, there's no duplication of ckeditor5-utils because two versions of the engine are happy with its never version.

I'm not 100% sure that npm/yarn will be able to figure out that this is the minimal installation setup, but it's entirely possible that they do or will do that.

Therefore, a separate check, unfortunately, would need to be included in every package that we want to secure. Every plugin and many engine/UI's base modules.

However, the second option is to check this in our webpack plugin. In fact, we planned that the runtime error will be just one of the verification mechanisms. The other was supposed to warn developers even earlier, when running webpack. At that stage, it's completely possible to tell that there are two versions of the engine used.

[Reinmar]

cc @pjasiun @oskarwrobel @ma2ciek

[ma2ciek]

AFAIK npm/yarn might work as you mentioned above and they can install e.g. two versions of the engine and only one version of ckeditor5-utils so our duplication check won't be executed.

Therefore, a separate check, unfortunately, would need to be included in every package that we want to secure. Every plugin and many engine/UI's base modules.

However, the second option is to check this in our webpack plugin. In fact, we planned that the runtime error will be just one of the verification mechanisms. The other was supposed to warn developers even earlier, when running webpack. At that stage, it's completely possible to tell that there are two versions of the engine used.

With webpack, we could do it without an impact on the size of the final bundle.

[ma2ciek]

Note to self - this should work for 3rd party plugin deps too.

[ma2ciek]

However, the second option is to check this in our webpack plugin. In fact, we planned that the runtime error will be just one of the verification mechanisms. The other was supposed to warn developers even earlier, when running webpack. At that stage, it's completely possible to tell that there are two versions of the engine used.

Moreover, with the webpack-based checking, we should be able to distinguish the reason why the packages are duplicated:

Scenario 1:
Adding source files with dependencies to CKEditor 5 to the CKEditor 5 build (we can check if the file is the CKEditor 5 build by some simple regexp checks on the file).

Scenario 2:
Building two editors with duplicated node_modules (e.g not flattened because of some ugly NPM bug).

Scenario 3:
Building two editors with packages in different versions.

What we can't check is: Adding two different builds on the same site. But actually we could allow adding many builds on the same page at some point as with the above check we can get rid of the runtime duplication check (?).

The large benefit I see here is that in each scenario we can provide very detailed information about how to get rid of the issue since we'll know the whole user's environment and the bug category.

TBH we can even collect simple anonymous data like a {id: 3, reason: '3rd-party-package-old-deps' } that could be sent from the client. Although I'm pretty sure it's a kind of a privacy violation 😄But maybe we could ask the client if he would like to send anonymous data.

Maybe we should increase the priority of this issue?

cc @Reinmar @wwalc

[CKEditorBot]

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

[CKEditorBot]

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).