Skip to content

Conversation

@simon360
Copy link

@simon360 simon360 commented May 1, 2019

This project uses typescript in its development - but by the time it's published to NPM, it's compiled down to vanilla JS. The @types packages which were specified in peerDependencies would be useful for consumers that use typescript, but:

  1. those users will already know to install those @types packages, because that's how React and
    Angular get their types, and
  2. the users who don't use typescript will get warnings

Those types are more accurately peer dependencies for React and Angular, and only when typescript is in use. They probably don't belong on this package, which simply uses whatever version of React and Angular it gets.

In order to kill off those extraneous warnings, I've removed those packages from peerDependencies. It should have no impact on the package itself, only the warnings produced when installed.

This project uses typescript in its development - but by the time it's published to NPM, it's compiled down to vanilla JS. The @types packages which were specified in
peerDependencies would be useful for consumers that use typescript, but: 1. those users will already know to install those @types packages, because that's how React and
Angular get their types, and 2. the users who _don't_ use typescript will get warnings Those types are more accurately peer dependencies _for React and Angular_, and _only
when tuypescript is in use_.

In order to kill off those extraneous warnings, I've removed those packages from peerDependencies. It should have no impact on the package itself, only the warnings produced
when installed.
@bcherny
Copy link
Contributor

bcherny commented Jun 15, 2019

TypeScript doesn't have a great story here. Ideally, we'd detect when the consumer is using TS and install peerTypes automatically (Yarn is moving in that direction). Until then, I'd prefer to keep this as-is: the trade-off is hard-to-debug errors for TS users vs. ignorable warning messages for non-TS users; my hunch is it's better to optimize for the former in this case.

Thanks for taking the time to contribute, and sorry if this isn't the outcome you wanted!

Copy link
Contributor

@bcherny bcherny left a comment

Choose a reason for hiding this comment

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

^

@simon360
Copy link
Author

Thanks for the response! Obviously, that's your perogative. That being said... if you don't mind, I'd like to present a counter-argument, if for no other reason than a sense of completeness on this PR.

The first one is on ignorable warnings. I respect the sentiment of what you're saying - especially in this case, the warnings can be safely ignored. However, if a project begins accumulating warnings, then all warnings begin getting ignored, simply on human nature. If running yarn presents 5 lines of yellow text on a project, then it's easy to miss when that goes up to 6 lines, or 7.

In the case of react2angular, these peer dependencies are optional. However, most packages that specify peer dependencies are doing so because the dependencies are absolutely critical to the package, but the version should always be deferred to whatever's provided by the install location. If what's provided doesn't match the version range, then a warning pops up. Having the 4 warnings from react2angualr here, makes those warnings harder to discover.

The second one is on the utilitty. I'd suggest that the vast, vast majority of projects that use Angular or React, in conjunction with TypeScript, will already have @types installed for those packages. Tools like tslint regularly throw up suggestions that they be installed.

There's nothing about react2angular that makes those types more valuable, as far as I can tell. If a project doesn't have the types packages installed, it'll be harder to debug, regardless of whether that project uses react2angualr or not. Specifying the @types packages as peer dependencies may be making a good case that the packages are useful, but it's making it to a very small audience.

In short: most TypeScript users will already have those @types packages installed. A very, very small audience won't. However, very few of the non-TypeScript users will have those @types packages installed, and none of them should need it. Effectively, my feeling is that this specification of peer dependencies is having a small positive effect on a small number of people, and having a small negative effect on a larger number of people. Weighing that equation suggests that it might be worth removing the peer dependency specification.

Again, totally up to you - this isn't my project, and your reasons aren't necessarily invalid. Sorry for writing a bit of an essay here, and I hope it makes sense.

@monospaced
Copy link

I might be missing something here, but Including dependencies that are only used for development as peerDependencies does not seem right.

It communicates to the consumer of the package, that something is missing, which they need to install. But really they don't, as they are consuming transpiled ES5 not TypeScript(?)

Perhaps adding information about the necessary @types/* versions in a Contributing section of the README could be an altertnative compromise?

@piehouserat
Copy link

guys, I know this is fairly minor but can this not be merged? We also do not use typescript and yet get this warning. I'd ideally like to get rid of all warnings for my project from the console but this is a blocker

@simon360
Copy link
Author

simon360 commented Apr 2, 2020

Going to close this. Seems like it'll never be merged, so might as well declutter.

@simon360 simon360 closed this Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants