ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.54k stars 3.7k forks source link

Missing peer dependency webpack on dev-utils package #14820

Closed jnoordsij closed 1 month ago

jnoordsij commented 1 year ago

πŸ“ Provide detailed reproduction steps (if any)

  1. Run (empty directory): yarn set version berry
  2. yarn add -D @ckeditor/ckeditor5-dev-utils
  3. The following messages will appear:
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (p9dd4f), requested by css-loader
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (pc8460), requested by esbuild-loader
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (p22d4b), requested by mini-css-extract-plugin
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (p28963), requested by postcss-loader
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (pe35ec), requested by raw-loader
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (p06d68), requested by style-loader
    ➀ YN0002: β”‚ @ckeditor/ckeditor5-dev-utils@npm:38.4.0 doesn't provide webpack (p76864), requested by terser-webpack-plugin

βœ”οΈ Expected result

No output.

❌ Actual result

Error messages by Yarn.

❓ Possible solution

Add appropriate versions of webpack as (optional) peerDependencies to package.json.

πŸ“ƒ Other details


If you'd like to see this fixed sooner, add a πŸ‘ reaction to this post.

Witoso commented 1 year ago

CC @pomek.

Reinmar commented 1 year ago

Is webpack a strict dependency of all utils available in dev-utils package? If not – IMO it's fine that it's not listed as a peerDependency. It may be a dependency of an app that uses dev-utils but not of dev-utils itself.

Sorry for being so rejective of the warnings logged by yarn but we have so much headache here with all package managers behaving differently that I'm now on the "don't touch if it seems to work" side.

jnoordsij commented 1 year ago

It's only a dependency for part of the tools AFAICS. I think it shouldn't cause any problems adding it as a peer dependency marked as optional (see https://docs.npmjs.com/cli/v9/configuring-npm/package-json#peerdependenciesmeta), but I do understand different package managers might behave differently and that's an annoying thing to deal with.

For reference, this warning can be silenced locally by force-adding this dependency locally in your .yarnrc.yml file:

packageExtensions:
  '@ckeditor/ckeditor5-dev-utils@*':
    peerDependencies:
      webpack: "*"
    peerDependenciesMeta:
      webpack:
        optional: true
pomek commented 1 year ago

ckeditor5-dev-utils does not use webpack directly. There is only one mention of the package in the entire project:

It may be a dependency of an app that uses dev-utils but not of dev-utils itself.

☝️ Exactly.

If we add webpack as peer deps, our Vite integration will be installing webpack (due to release tools using utils).

jnoordsij commented 1 year ago

If we add webpack as peer deps, our Vite integration will be installing webpack (due to release tools using utils).

I don't understand this; I think the whole point of a peerDependency is notifying the package consumer which version should be installed with a certain version constraint in their dependencies, to ensure matching versions, and thus it should never be installed by default.

I think this blog explains the point of why to mark webpack as peer dependency pretty well, altough I think in practice it doesn't really matter here.

Reinmar commented 1 year ago

Starting from npm v7, npm installs peerDependencies automatically. And, which isn't necessarily a problem in this specific case, the algorithm it uses to do so causes issues because for odd reasons it installs a wrong version of a package (duplicating already installed packages). This makes us very cautious when playing with peerDependencies.

We don't have a single ticket now where we handle these topics. There's a chance we'll find some reasonable resolutions to these issues (#13344 + one that's handled in our private repo for the time being). But for now, we're cautious.

jnoordsij commented 1 year ago

Ah I see, though a little surprising to me I kind of understand why they'd go with this.

I did however find a follow-up RFC to the one enabling the auto installation of peer dependencies, which disables auto-install for optional ones. So maybe marking it as optional (which it is) will work for all cases.

CKEditorBot commented 2 months ago

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 still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

CKEditorBot commented 1 month ago

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