-
Notifications
You must be signed in to change notification settings - Fork 148
Allow using dev dependencies in a package json #655
base: master
Are you sure you want to change the base?
Conversation
…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
There was a problem hiding this 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.
src/lib/definition-parser.ts
Outdated
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Of the set of 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 ) |
Do note that it is in npm's roadmap to install But yes, for nearly everything in DefinitelyTyped, |
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 |
In all fairness, the ultimate culprit is typescript eagerly loading every single |
What about making these devDependencies instead? They're not installed by default. |
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. |
Thanks for taking up this issue @orta! 😄
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 It's not perfect: RN users might also have to add some config ( |
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 |
I've updated the docs, and I'll poke Ron in chat |
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:
Edit: |
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.
|
The idea here is, inside 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 "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 "devDependencies" is the only kind of dependency left, and that sounds a little weird at first, but it has the behavior we want; if It's a little bit weird, but then this whole problem is a little bit weird, because |
Comment related to this: npm/rfcs#43 (comment)
|
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. |
Ok, (sorry for the rapid-fire replies here) I think the answer is a lot simpler.
Here's what I'd do. Put it in Another option is to add a |
If today's
Excuse me, but this isn't about including code. This is about type definitions and the presence thereof - there's nothing to |
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. |
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.) |
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?
Not everything has to be delivered via a package manager ;) |
Remove
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 |
And let's not forget that npm/npm#3066 was closed and locked in 2015 with the explaination that
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 |
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 |
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 |
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 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. |
I couldn't find anything indicating |
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. |
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?
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. |
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:There's a few whitespace and tslint import warnings I fixed on my way through the codebase too