Skip to content
This repository was archived by the owner on Nov 20, 2023. It is now read-only.

deps: add missing peerDep #343

Merged
merged 1 commit into from
Jan 25, 2022
Merged

deps: add missing peerDep #343

merged 1 commit into from
Jan 25, 2022

Conversation

m1heng
Copy link
Contributor

@m1heng m1heng commented Jan 25, 2022

add webpack as peerDependency as webpack is used in index.d.ts for typing

import { Compiler, WebpackPluginInstance } from 'webpack';

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@kamilogorek kamilogorek merged commit a49e061 into getsentry:master Jan 25, 2022
@rchl
Copy link
Contributor

rchl commented Feb 8, 2022

I'm not sure about that change... Not sure if when the usage of a dependency is only within types then that warrants a peer dependency.

This forces NPM to install the webpack dependency in projects that use @sentry/webpack-plugin. And if the @sentry/webpack-plugin is a production dependency then webpack is also installed in production, adding to the weight of the artifact.

Types are not really a requirement for using this package, thus I'm not sure if this is really a correct change.

You could say that if I don't want webpack included in production then I should not install @sentry/webpack-plugin as a production dependency. But what about the case when using @nuxtjs/sentry package? It currently comes with @sentry/webpack-plugin as a production dependency. Another option would be to mark @sentry/webpack-plugin as a peer dependency there too but then I would have again the same problem there -- since the @nuxtjs/sentry is a production dependency then both @sentry/webpack-plugin and webpack would be installed as a production dependencies.

I'm not sure what are the typical conventions in those kind of scenarios but something feels off about this change due to how NPM handles such cases.

@rchl
Copy link
Contributor

rchl commented Feb 8, 2022

I guess I could also add an argument that Webpack 4 doesn't come with types (the types are provided through @types/webpack) so when using version 4, it's not even necessary to have it installed.

I would opt for reverting this change and instead maybe mentioning this in the documentation since the situation is complicated with webpack.

@m1heng m1heng deleted the fix/peer branch February 9, 2022 02:51
@kamilogorek
Copy link
Contributor

Sounds reasonable, however, it would be still somewhat useful to provide this information to the end user.
One way to do this would be to use peerDependenciesMeta and mark webpack as optional. WDYT?

@rchl
Copy link
Contributor

rchl commented Feb 14, 2022

Based on my past experience, I don't think that would work. The npm would still install the dependency as a production one. It would just not complain if it failed installing.

@AbhiPrasad
Copy link
Member

Based on my past experience, I don't think that would work. The npm would still install the dependency as a production one. It would just not complain if it failed installing.

As per npm/rfcs#221 (comment), setting optional: true will make sure the peer dependency does not get automatically installed.

I am in favour of using peerDependenciesMeta with optional true here.

@rchl
Copy link
Contributor

rchl commented Feb 14, 2022

I'm all for trying and would be glad if it worked .

@kamilogorek
Copy link
Contributor

Releasing as 1.18.6 (will be out once getsentry/publish#825 is approved)

@rchl
Copy link
Contributor

rchl commented Feb 15, 2022

As I expected, this didn't really help my case.

NPM now treats the webpack dependencies as devOptional rather than dev:

Screenshot 2022-02-15 at 12 23 44

But all of those are still installed on installing only production dependencies (npm ci --production).

@rchl
Copy link
Contributor

rchl commented Feb 15, 2022

Note that webpack dependency exists in the project since it's a Nuxt project so it comes with webpack. I guess it wouldn't be a problem if webpack would not be present in the first place.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants