Skip to content

Configure svelte as an optional peer dependency #279

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

Closed
theurgi opened this issue Feb 14, 2023 · 7 comments · Fixed by #294
Closed

Configure svelte as an optional peer dependency #279

theurgi opened this issue Feb 14, 2023 · 7 comments · Fixed by #294
Labels
enhancement New feature or request

Comments

@theurgi
Copy link

theurgi commented Feb 14, 2023

Description

Does it make sense to mark svelte as an optional peer dependency as it is for eslint-plugin-svelte?

I have an eslint config that conditionally depends on svelte and its plugins, so in some cases it's not installed. However, I can't seem to suppress pnpm's missing peer deps warning with any configuration on my end.

I assume that pnpm's output indicates that the warning is propagating upwards from svelte-eslint-parser:

 WARN  Issues with peer dependencies found
.
└─┬ @theurgi/eslint-config 1.3.0
  ├── ✕ missing peer svelte@"*"
  └─┬ eslint-plugin-svelte 2.18.0
    ├── ✕ missing peer svelte@^3.37.0
    └─┬ svelte-eslint-parser 0.23.0
      └── ✕ missing peer svelte@^3.37.0
Peer dependencies that should be installed:
  svelte@">=3.37.0 <4.0.0"

In my package: @theurgi/eslint-config, I have svelte set as optional:

"peerDependenciesMeta": {
  "svelte": {
    "optional": true
 },

But this does not silence the warning when using pnpm. Any package that depends on my shareable config inherits this warning.

Interestingly, I get no warning when installing with npm. I haven't tried yarn.

Thank you 🙏

@theurgi theurgi added the enhancement New feature or request label Feb 14, 2023
@ota-meshi
Copy link
Member

Does it make sense to mark svelte as an optional peer dependency as it is for eslint-plugin-svelte?

I think it makes sense.
I think you should ignore the warning or install svelte.

@theurgi
Copy link
Author

theurgi commented Feb 15, 2023

Thanks for your reply.

What I mean is, does it make sense to add peerDependenciesMeta.svelte.optional: true to svelte-eslint-parser's package.json? I'm happy to submit the pull request if you don't consider it an undesirable configuration.

Because, unless I'm mistaken, that is where the warning stems from. There seems to be nothing more I can do within my package to suppress this warning.

Specifically, it appears that the warning propagates from svelte-eslint-parsereslint-plugin-svelte@theurgi/eslint-config.

Though both eslint-plugin-svelte and @theurgi/eslint-config have peerDependenciesMeta.svelte.optional: true set, this does not suppress the warning.

The other option is to simply remove svelte from peerDependencies within both svelte-eslint-parser and eslint-plugin-svelte; for example, eslint-plugin-vue does not have vue as a peer dependency and thus it produces no warning. The same is true of eslint-plugin-react.

But I don't know if removing the peer dependency on svelte is feasible for this project.

If there's no other recourse, I'll have no choice but to ignore the warning but it was worth a shot to try to resolve this issue at its root (if indeed this is the root of the issue).

@ota-meshi
Copy link
Member

The parser depends on the svelte package. The parser will not work if svelte is not present.
If you can create a similar parser that doesn't depend on the svelte package, please create and publish it.

@theurgi
Copy link
Author

theurgi commented Feb 16, 2023

The parser will not work if svelte is not present.

Of course. I did not suggest otherwise. I expect the parser not to work when svelte is not present and that is precisely the case I'm referring to.

To clarify my use case:

@theurgi/eslint-config depends on eslint plugins for multiple frameworks and conditionally extends them based on file extension and whether the framework package ( in this case svelte) is a dependency of the project. This allows for a single shareable config that works across all projects, capable of linting any combination of tooling: Svelte with TypeScript; Svelte without TypeScript; React with and without TypeScript; An Astro monorepo with JSX, Vue, Svelte, and Prettier; and so on...

This approach is in contrast to the popular eslint-config-alloy which asks the user to...

Please choose the following configuration based on the technology stack used by your project:

  • Built-in
  • React
  • Vue
  • TypeScript
  • TypeScript React
  • TypeScript Vue

... and then requires them to install and configure, for example:

npm install --save-dev @babel/core @babel/eslint-parser @typescript-eslint/eslint-plugin @typescript-eslint/parser @vue/eslint-config-typescript eslint eslint-config-alloy eslint-plugin-vue vue-eslint-parser

In addition to linting multiple frameworks,@theurgi/eslint-config also resolves this Support having plugins as dependencies in shareable config issue without having to wait for the new ESLint config succeed its experimental phase.

Don't get me wrong, I'm not here to advocate for my personal eslint configuration preference. I'm just trying to describe a scenario where one may want to install, but not necessarily use, a tool (svelte-eslint-parser) which has a peer dependency (svelte) yet does not want to satisfy that peer dependency and would like the ability to suppress its irrelevant warning which otherwise propagates to all dependents.

The parser depends on the svelte package.

Strictly speaking, it does not. It has a peer dependency of the svelte package which is a deferral to the user to satisfy the dependency.

But of course, it is assumed to be the case that if a user is installing svelte-eslint-parser they are also installing svelte.

Nevertheless, listing svelte as a peer dependency is relevant because it specifies a version range.

And that is why my request was to first add peerDependenciesMeta.svelte.optional: true. With npm and this setting, I can suppress the warning myself by adding it higher in the tree. But with pnpm, apparently I can not; it seems to propagate the warning regardless and instead offers as a solution pnpm.peerDependencyRules.ignoreMissing. But this is not a publishable setting, it has to be added in the package manifest of every dependent project. Suboptimal.

If you can create a similar parser that doesn't depend on the svelte package, please create and publish it.

Is this a necessary statement?

I admire and appreciate your work. I realize you may consider this a trivial matter. If my argument to make this non-functional change has not convinced you and you disagree, that is fine, just say so. I'm not sure what GitHub issues are for if not to deliberate on collaborative source code and to collectively reap the benefit of doing so.

@ota-meshi
Copy link
Member

I'm not sure what you mean by this issue. (It may be because I don't understand English very well 😓)
This package depends on the svelte package. If it's a problem to be warned, I think you should install the svelte package. Are there any other solutions?

@theurgi
Copy link
Author

theurgi commented Feb 16, 2023

Forgive me if I was not clear.

There is a difference in the way that npm and pnpm emit warnings in regards to missing peer dependencies.

A demonstration

npm

image

As you can see, no warnings are emitted.

pnpm

image

And here we have warnings despite the fact that all packages installed are identical to those installed with npm.

Why I consider this a problem

The context

As I described in my previous comment, @theurgi/eslint-config extends, and strictly depends on, eslint plugins for React, Vue, Svelte, TypeScript, Astro, Prettier, and more.

The benefit of doing this is that with a single shareable eslint config (only one package to maintain, only one package to install), I can lint any combination of JavaScript tooling.

I discovered this architecture here: privatenumber/eslint-config and of all the ways I have seen to create a shareable eslint config, this, in my opinion, is the best.

The problem

I intend to use @theurgi/eslint-config in various different JavaScript projects. But, when the project I am working on is, for example, a Next.js/React project, I would prefer not to have warnings emitted about a missing svelte peer dependency every time I, or someone else, runs pnpm install ....

And I certainly don't want to install svelte and have it listed as a dependency in the package.json of a Next.js/React project just for the sake of suppressing this warning—that would be very misleading.

How I've attempted to address this issue

Initially, I opened this issue with pnpm.

Despite what the maintainer of pnpm proposed as a solution, I was not able to suppress warnings emitted by dependencies of @theurgi/eslint-config by including any combination of the following in its package.json.

"peerDependencies": {
  "eslint": "*",
  "svelte": "*"
},
"peerDependenciesMeta": {
  "svelte": {
    "optional": true
}

My next logical assumption was that with pnpm, missing dependency warnings must be suppressed at their root: i.e. svelte-eslint-parser.

Considering that the maintainer of pnpm dismissed my issue, my next option was to see if you, the maintainer of this project, would be willing to include the above code in your package.json. I did not know at the time whether this would work; I was partially asking if you knew, and partially asking if you would consider testing it.

Where we are now

After additional tests, I am now convinced that even if you did include peerDependenciesMeta.svelte.optional: true in svelte-eslint-parser's package.json, it would not suppress the warning. It doesn't seem to matter where this property is set in the dependency tree.

Therefore the only option left to suppress such warnings is to include the following in the package.json of every project that depends on @theurgi/eslint-config:

"pnpm": {
  "peerDependencyRules": {
    "ignoreMissing": ["svelte", "..."]
  }
}

This is not something I can include in @theurgi/eslint-config's package.json itself—pnpm publish removes all pnpm properties from published configs. Furthermore, it only reads this setting in a project's root package.json.

This was the least desirable option to fix this issue in my opinion and I wanted to exhaust all other options before resorting to it.

That is all.

So, thank you again for your open source contributions. Thank you for your time and consideration. Hopefully our exchange was not entirely futile for you. And judging by the documentation you have written, your English is very good.

🙏

@ota-meshi
Copy link
Member

ota-meshi commented Feb 16, 2023

Thank you for explaining it all over again!
I must apologize. I had a very big wrong understanding.
I totally thought that svelte-eslint-parser already included peerDependenciesMeta. But that was not the case. We need to add peerDependenciesMeta to svelte-eslint-parser.

  "peerDependenciesMeta": {
    "svelte": {
      "optional": true
    }
  },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants