Skip to content
This repository has been archived by the owner on Jun 11, 2020. It is now read-only.

Allow using dev dependencies in a package json #655

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

orta
Copy link
Contributor

@orta orta commented Sep 14, 2019

The problem: Styled Components (and other node + RN projects) wants to augment types which come from React Native. The thing is you can't have @types/node and @types/react-native in the same repo because they have conflicting types.

What this has meant is that everyone using styled-components with web uses an old version of @types/styled-components, and the react native folks can use the latest. That kinda sucks.

A possible solution: Letting a library set a dev dependencies in the package.json for their section of DT. Then, during parsing that dependency is removed from "dependencies" in the npm version of the types and set in "devDependencies".

Using this technique would reduce the dependencies for styled-components from "react", "react-native" and "csstypes" to "react", "csstypes". Allowing people to have those features if they want, but not break builds.

I tested locally that it still works when @types/react-native is removed:

mkdir sc-test
cd sc-test
yarn init --yes; yarn add typescript  @types/node @types/styled-components
yarn tsc --init
yarn tsc --noEmit # Found 19 errors.

rm -rf node_modules/@types/react-native
yarn tsc --noEmit # Success

There's a few whitespace and tslint import warnings I fixed on my way through the codebase too

…pendency doesn't have to be included for the package to pass - this is because styled-components infers react-native, but it doesn't need it to pass
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Seems like a good idea.

One question: I thought peer dependencies were still real dependencies, but ones whose versions might vary in dependents. That doesn't seem to be the meaning here; it seems like a way to avoid installing one of two packages that would conflict at compile-time. I may be confusing peer dependencies with something else, though, or may just be confused.

Also, I'd like @rbuckton to sign off too since he's thought a lot about versioning and may think of something I've missed.

const license = getLicenseFromPackageJson(packageJson.license);
const packageJsonDependencies = checkPackageJsonDependencies(packageJson.dependencies, packageJsonName);
const packageJsonDependencies = checkPackageJsonDependencies(packageJson.dependencies, packageJsonName, /* checkWhitelist */ true);
const packageJsonPeerDependencies = checkPackageJsonDependencies(packageJson.peerDependencies, packageJsonName, /* checkWhitelist */ false);
Copy link
Member

Choose a reason for hiding this comment

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

why do peer dependencies get a pass from the whitelist?

Copy link
Contributor Author

@orta orta Sep 16, 2019

Choose a reason for hiding this comment

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

I originally wrote "react-native" in the peerDependencies - and that isn't whitelisted because it exists inside @types.

I originally wrote almost the same function, but realized if I could skip the whitelist then it'd be the same code.

I'm open to the idea that @types/react-native" should be the bit in peerDependencies instead. Then I'd have to do a bit of work to remove the @types when filtering also, but that could be an ok trade-off instead.

@orta
Copy link
Contributor Author

orta commented Sep 16, 2019

Of the set of "xDependencies" in a package.json - peer dependencies are the only ones which aren't installed by default. This tends to mean that people use it as a weak link (which is why it's example is usually the plugin system where you don't want two copies in the tree for no reason)

So, it's not a "real" dependency in the way that it actually downloads and installs them. In Artsy we used them to indicate the versions which downstream consumers could care about, but have their own choice in the exact version. ( In this package you might not need any of these deps, depending on what parts of the lib you were using )

@Jessidhia
Copy link

Do note that it is in npm's roadmap to install peerDependencies by default again, see https://github.com/npm/cli/wiki/Roadmap#proper-peerdependencies-support and the RFC at npm/rfcs#43.

But yes, for nearly everything in DefinitelyTyped, peerDependencies is actually the correct dependency type, and IIUC it only isn't used because of the convenience cost of forcing the user to install them manually.

@orta
Copy link
Contributor Author

orta commented Sep 19, 2019

install peerDependencies by default again

Ergh, that's a shame. Thanks @Jessidhia

OK, I think this will just have to be a way for a DT package to declare a blocklist for inferred dependencies in the @types/package version. No point in shipping this way and then having to revert in the future when it breaks.

@Jessidhia
Copy link

In all fairness, the ultimate culprit is typescript eagerly loading every single @types package installed unless you specify a compilerOptions.types. I was already a user of this whitelisting approach which is how I never noticed the react-native issues for a while...

@jwalton
Copy link

jwalton commented Sep 19, 2019

Of the set of "xDependencies" in a package.json - peer dependencies are the only ones which aren't installed by default.

What about making these devDependencies instead? They're not installed by default.

@orta
Copy link
Contributor Author

orta commented Sep 19, 2019

Aye, that's fair - but automatic type acquisition for JS projects end up needing the config-less option to be solid. Maybe devDependencies might have to be it, peer deps were useful because you could get a message. Ideally, messaging-wise optional dependencies feels the best.

@jkillian
Copy link

Thanks for taking up this issue @orta! 😄

OK, I think this will just have to be a way for a DT package to declare a blocklist for inferred dependencies in the @types/package version. No point in shipping this way and then having to revert in the future when it breaks.

I think that seems reasonable. Would also be nice if info could then be added to the package's README: 'If you want to use @types/styled-components with react native, please also install @types/react-native@^3.0.0' or something like that.

It's not perfect: RN users might also have to add some config (compilerOptions.types, .yarnclean) to make sure @types/node aren't messing up their typings, but at the least it lets us support the most popular usecase (web) and still make RN use possible.

@jwalton
Copy link

jwalton commented Sep 19, 2019

peer deps were useful because you could get a message. Ideally, messaging-wise optional dependencies feels the best.

If you're not a react-native developer though, you're also going to get a message every install complaining about missing peer dependencies. :P

@orta orta changed the title Allow using peer dependencies in a package json Allow using dev dependencies in a package json Oct 3, 2019
@orta
Copy link
Contributor Author

orta commented Oct 3, 2019

I've updated the docs, and I'll poke Ron in chat

@sandersn
Copy link
Member

sandersn commented Oct 3, 2019

Let's talk about this in the design meeting. We have 3-4 possible solutions to this and I think having the whole team's input would be useful.

A couple more ideas Ron and I came up with:

  1. Maybe we need a third dependency kind, "typeDependencies". But this would require a lot of change from npm and the ecosystem.
  2. On the flip side, all the instances I'm aware of this problem are with node, and almost all of them are for Buffer. it would be a lot easier to create a @types/node-buffer module that both @types/node and anybody else could depend on.

Edit:
3. Forward declarations of types would also solve this I think. We've run into trouble designing that before, though, as I recall.

@sandersn
Copy link
Member

sandersn commented Oct 4, 2019

Orta gave a good description of the problem above. Here's a summary of the solutions we've come up with so far. I'll update the list as more come in.

  1. Allow devDependencies in DT's package.json. This is the solution I understand the least. You can explicitly install the node dependency but it won't show up otherwise?
    a. Allow peerDependencies in DT's package.json. This doesn't install the node dependency (it may in the future), but you may have it from elsewhere if your project already uses node.
  2. Allow a third dependency kind in DT's package.json, "typeDependencies". Actually, I'm not sure how this solves the problem here -- it is great for packages written in Typescript avoid putting type-only dependencies in "dependencies", but doesn't help react-native coexist with node in a compilation.
  3. This only really happens for node types, and almost all the instances are for Buffer. If DT had a @types/node-buffer package, both node and react-native could depend on that.
  4. Forward declarations. react-native could forward declare Buffer without specifying its contents and only types named Buffer would be assignable to it. The declaration might be scoped in some way too.

@jwalton
Copy link

jwalton commented Oct 4, 2019

Allow devDependencies in DT's package.json. This is the solution I understand the least. You can explicitly install the node dependency but it won't show up otherwise?

The idea here is, inside @types/styled-components, we want to modify some types from @types/react-native. But, as a consumer of @types/styled-components, in node.js land you don't want @types/react-native installed, because they cause a bunch of type conflicts with node. So, @types/react-native can't be in the "dependencies" of @types/styled-components, because dependencies get installed automatically when you npm install in the package that references @types/styled-components. (Let's call this package that depends on @types/styled-components "referrer", just to give it a name.)

So what we need is a "dependency" that doesn't have this property. "peerDependencies" seems like a good fit here, because the module won't automatically be installed when you npm install in "referrer", except that a) you'll get a warning that there's an unmet peer dependency, and b) in npm 7 it's very likely that the module will, in fact, be installed automatically.

"optionalDependencies" has a nice ring to it, but it behaves like "dependencies" - the only difference is that if a package fails to build, then the package won't be installed. Since there's no build script for @types/react-native, it'll always be installed. (Could we add a build script to @types/react-native that tries to detect if @types/node is installed? Would that work reliably or would it depend on the order things are installed in? Probably not a wonderful idea.)

"devDependencies" is the only kind of dependency left, and that sounds a little weird at first, but it has the behavior we want; if @types/react-native is in "devDependencies" for @types/styled-components, it will not be installed when we do an npm install in "referrer" (just like babel, mocha, etc... are not installed when you install a library that depends on them). So if you're in a node project, the react-native types won't be installed. If you're in a react-native project, and you want to use @types/styled-components, you very likely already have @types/react-native installed, and if you don't you can certainly explicitly add it, so it works there too.

It's a little bit weird, but then this whole problem is a little bit weird, because @types/react-native isn't really a dependency of @types/styled-components. styled-components doesn't "need" react-native at all, it wants to add things to react-native but only if it's already there. It doesn't depend on it, so much as it "augments" it. (Maybe instead of adding a typeDependencies: to package.json, if we want to go down that road, we should add an augments: to package.json - a list of packages this package modifies if they are present?)

@alexanderjarvis
Copy link

@isaacs
Copy link

isaacs commented Oct 25, 2019

Comment related to this: npm/rfcs#43 (comment)

Using peerDependencies as an "optional dep not installed by default" is definitely a kludge that no one should be relying on. Turning off default peer dep installation was a mistake, and was never intended to be permanent. DefinitelyTyped should certainly not rely on that behavior being portable, since @arcanis has indicated that yarn will not match npm's behavior on this.

I would not be opposed to a dep type or flag to indicate that package's optional deps (and optional peerDeps) should not be installed by default. (Sort of like npm's --no-optional flag, but targeted to a single package's deps.)

@isaacs
Copy link

isaacs commented Oct 25, 2019

It'd be good to be able to clarify exactly what DT needs in a dependency type, and then evaluate what gets the closest today, and how we could improve that in the future. Relying on peerDeps not being installed is a bad idea.

@isaacs
Copy link

isaacs commented Oct 25, 2019

Ok, (sorry for the rapid-fire replies here) I think the answer is a lot simpler.

styled-components doesn't "need" react-native at all, it wants to add things to react-native but only if it's already there.

Here's what I'd do. Put it in devDependencies so that you can have a test that verifies the behavior. Don't put it anywhere else. At run-time, load it in a try block, and do the right thing if it's found. Basically, treat it as an unlisted dep (albeit, one that's loaded responsibly with the expectation that it might not be found).

Another option is to add a postinstall script to react-native to check for the node types package and raise a conflict error, and have react-native listed as an optionalDependency or peerDependency with peerDependenciesMeta setting it to optional:true.

@weswigham
Copy link
Member

weswigham commented Oct 29, 2019

It'd be good to be able to clarify exactly what DT needs in a dependency type, and then evaluate what gets the closest today, and how we could improve that in the future. Relying on peerDeps not being installed is a bad idea.

peerDependencies are the closest thing to what is needed today. Here's what's needed:

  1. Not installed by default. (like peerDependencies today)
  2. No errors if not installed. (like peerDependencies almost is today - it warns non-critically, and it would be nice if the warning were removable)
  3. Validates version match if they are installed as a peer (like peerDependencies today)

If today's peerDependencies was simply renamed to and understood to be optionalPeerDependencies we'd be much better off for it. (And TBH, I think a much better fix, @isaacs, than installing peer deps would just be removing the warning on today's peerDependencies entirely and adding a requiredPeerDependencies that behaves like you suggest peerDependencies should in npm/rfcs#43 - this'd minimize breakage and confusion in the ecosysem)

At run-time, load it in a try block, and do the right thing if it's found. Basically, treat it as an unlisted dep (albeit, one that's loaded responsibly with the expectation that it might not be found).

try block

try

Excuse me, but this isn't about including code. This is about type definitions and the presence thereof - there's nothing to try - either the file is there or it isn't. What we'd like to say is that it's up to the consumer to determine if some other package needs to be present and, if it is, that its version is compatible. There's no code involved, other than npm or another package manager validating and installing someone's project.

@weswigham
Copy link
Member

weswigham commented Oct 29, 2019

The challenge for us, testing-wise, is if we go with.. well... any of @sandersn and @orta's solutions, is determining a reasonable test matrix for a project that utilizes them. @types/styled-components is a great example - we probably need to test alongside both @types/node and @types/react-native, but not both simultaneously. It's a very specific and odd constraint that can't quite be captured in a versioning or peering requirement. It's a kind of oneof thing that doesn't exist really anywhere - I think we'd need to setup some special manifest fields and test infrastructure to handle this well.

@isaacs
Copy link

isaacs commented Oct 29, 2019

And TBH, I think a much better fix, @isaacs, than installing peer deps would just be removing the warning on today's peerDependencies entirely and adding a requiredPeerDependencies that behaves like you suggest peerDependencies should in npm/rfcs#43 - this'd minimize breakage and confusion in the ecosysem

As discussed in npm/rfcs#43, the problem with adding a new kind of peerDeps that are installed by default is that most packages using peerDependencies today don't work at all if their peer deps are not found, and work improperly if their peer deps get installed in as a direct dep.

Adding a new thing that behaves as most users are currently using the existing thing isn't as good as making the existing thing behave like people are using it. Making the warnings go away without making the thing the warnings are warning about go away is worse than our current state.

Also, a new kind of peerDeps that are installed by default will be completely ignored by npm v6 and earlier, which presents another problem (albeit one that might only last 2-5 years or so). (While we could theoretically implement that as a non-breaking change, doing so with the current installer codebase would be prohibitively difficult.)

@isaacs
Copy link

isaacs commented Oct 29, 2019

Excuse me, but this isn't about including code. This is about type definitions and the presence thereof - there's nothing to try - either the file is there or it isn't. What we'd like to say is that it's up to the consumer to determine if some other package needs to be present and, if it is, that its version is compatible. There's no code involved, other than npm or another package manager validating and installing.

Gotcha. It sounds like "unlisted dep that you investigate (or don't) at run time" is the way to go then?

For testing, if it's just about verifying that things work in the presence of different files, why not just bundle it as a test fixture?

It's a very specific and odd constraint that can't quite be captured in a versioning or peering requirement.

Not everything has to be delivered via a package manager ;)

@weswigham
Copy link
Member

Adding a new thing that behaves as most users are currently using the existing thing isn't as good as making the existing thing behave like people are using it. Making the warnings go away without making the thing the warnings are warning about go away is worse than our current state.

Remove peerDependencies entirely then, and replace it with two new dependency kinds (one optional, one not), whose behavior you don't change. Reverting to what is essentially npm 3 behavior isn't going to make the stackoverflow confusion go away, it's going to be magnified, as now you'll have some packages expecting pre-npm-3 behavior, and some packages expecting npm 4-7 behavior. There is no "winner" here, there is no "better" or "intended" behavior. It's definitely churn to silence a few loud voices that'll definitely come back with more equally loud voices who were broken by it. npm's peerDependencies have behaved as they do today for 4 years. 4 years. If you can admit that the core of the issue was the change over in npm 3, swapping back at this point is absolutely not going to solve the problem, since now there's 4 years of packages written with other behavior in mind.

Gotcha. It sounds like "unlisted dep that you investigate (or don't) at run time" is the way to go then?

Then there's nothing validating version range compatibility, which is remarkably important to ensuring things are deduped correctly, which impacts checking (as dupes are nominally separate!). We need something that behaves like peerDependencies does today. Ideally with less warnings.

@weswigham
Copy link
Member

And let's not forget that npm/npm#3066 was closed and locked in 2015 with the explaination that

Given that peerDependencies at most warn in npm@3, and given that npm no longer causes peerDependencies to be installed, in effect all peerDependencies are optional. I'm not exactly declaring victory and going home here, but with the current behavior, I think there's nothing actionable remaining to this issue, so I'm closing it.

and is the top google search result for "optional peer dependencies", I think you are severely underestimating how breaking npm/rfcs#43 might be. Relying on the current behavior has been the implicit guidance of npm for 4 years.

@isaacs
Copy link

isaacs commented Oct 29, 2019

I think you are severely underestimating how breaking npm/rfcs#43 might be.

I am not going to rehash npm/rfcs#43 all over again here. As stated in that issue, I'm open to reopening it if there are new concerns to consider. Otherwise, there's no need to speculate about "how breaking" it'll be. npm v7 will go through a lengthy beta period, likely including programmatically installing everything in the npm registry, and that process will shake out empirically how much of a good or bad idea it is, along with other changes in that version.

When we have more data, we'll obviously consider it. It could be that peerDeps are made actually optional and ignored by default (by removing the confusing warning, and perhaps adding a new kind of peerDeps that install by default), or that we reserve the spot for them but otherwise do nothing unless the user opts in with a flag, or warn that they will become installed by default in v8. All those changes are easy, but without having the capability of creating a correct tree with peer deps, we are stuck in the current unfortunate state.

Best data available to me today indicates that people who are intimately familiar with package/module system internals are split on installing peer deps by default, and most others are in favor of it. I am not swayed by the intensity of your emotion about it nearly as much as I am by data about which programs and use cases will benefit, and I care much more about people running npm install than the people implementing it. I'm confident that we will want the option to install peer deps for the user easily and correctly, even if it's not done by default, so we're building the capability in v7, which also will allow us to collect better data. Until then, I recommend getting upset about other things instead. But I would advise that you not rely too strongly on peerDeps not being installed by default.

@weswigham
Copy link
Member

Well, as I said, there is no other dep kind that fills this use, and the npm issue requesting a dependency kind that works like this was closed four years ago, stating that peerDependencies could be used in this way. I should hope you'll at least reopen that issue and reconsider it, then. Even disregarding the break, changing the behavior like the rfc proposes removes a useful dependency kind that we still have use for (perhaps obviously, since we're using it).

@arcanis
Copy link

arcanis commented Oct 29, 2019

Otherwise, there's no need to speculate about "how breaking" it'll be

That's exactly what an RFC is for. We're in this thread because the mere existence of your RFC makes at least one project (and probably more) confused how they should use a feature, where everything was agreed upon before - maybe not in your head, but certainly in everyone else's mind.

Overall, what irks me is that there's already a way to annotate mandatory peer dependencies. Now that we've implemented peerDependenciesMeta everywhere, we have a standard way to add metadata to existing peer dependencies. You could suggest a new field. You could even suggest to extend the already-existing "optional": true flag¹ to also support false as well. You could suggest to deprecate peer dependencies with no explicit optional field at publish time, and let the ecosystem self-correct. You could find a migration plan that would get our support.

Instead, the only option you seem willing to consider is an all-or-nothing plan. Your RFC offers little credible alternatives² and contains very little risk analysis - not even basics like "how many packages in the top 1000 use peer dependencies as optional tags?". It also doesn't describe obvious interactions, such as when two dependencies have similar peer dependencies that cannot overlap. And of course, it doesn't address the portability concern - if npm is the odd duck, won't that just increase confusion amongst your users instead of easing it?

You can tell us you're as data-driven as you want, but so far the only data you provided is a handful of results from a StackOverflow search on "peer dependencies". That's simply not enough to warrant breaking our users' expectations, and that makes me wonder what's the actual motivation.


¹ btw @orta @weswigham, this is what you're looking for. This field is supported by all package managers and will silence any warning when the dependency isn't there (while still printing one in case of mismatched versions).

² "Drop Support for Peer Dependencies Entirely" isn't even remotely possible, "Treat Like Regular Dependencies" isn't an option since the semantic is entirely different, "Treat Like Optional Dependencies" is irrelevant since optional dependencies impact the build, not the resolution.

@weswigham
Copy link
Member

this is what you're looking for. This field is supported by all package managers and will silence any warning when the dependency isn't there

I couldn't find anything indicating npm supported it? (Though yes, yarn and pnpm do, so we should consider setting it to silence warnings there, anyway, but npm still needs equivalent, especially if it's doing npm/rfcs#43)

@arcanis
Copy link

arcanis commented Oct 29, 2019

I implemented it in npm in npm/cli#224, and it got shipped along with the 6.11. There was a small server-side bug remaining, but as of today it should be fixed for future releases.

@isaacs
Copy link

isaacs commented Oct 31, 2019

@arcanis

You can tell us you're as data-driven as you want, but so far the only data you provided is a handful of results from a StackOverflow search on "peer dependencies". That's simply not enough to warrant breaking our users' expectations, and that makes me wonder what's the actual motivation.

I provided more data in the RFC, and explained further there. The more compelling data points are people reaching out via npm support email, on npm.community, conversations I've had with users directly, and my observations of experienced node.js users asking on Twitter why this doesn't work as they expect.

Please stop misrepresenting my position in this way. I understand that you disagree, but characterizing thousands of person-hours of user research and support work (not to mention hundreds of other SO threads) as a "handful of results on StackOverflow" is disingenuous and offensive. I provided 8 links to stack overflow (and suggested you could search yourself to easily find many more) when you said that it "addresses no particular pain point you had seen reported". Having seen the reports, you dismiss them. So...? Idk?

Instead, the only option you seem willing to consider is an all-or-nothing plan.

All I can do is redirect you to #655 (comment)

What part of that reads as "all or nothing" to you? The RFC hasn't yet been ratified, largely because of the issues you raised, and the v7 beta process will give us a chance to evaluate these concerns empirically. I'm kind of at a loss as to what you actually want or expect from me here.

I am going to stop following this thread, because it's getting repetitive and I cannot be confident that you are participating in good faith.

My email address is i@izs.me if anyone wants to continue the discussion productively.

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

Successfully merging this pull request may close these issues.

None yet

9 participants