Pull in types from DefinitelyTyped #1491
Conversation
| "lib" | ||
| ], | ||
| "engines": { | ||
| "node": ">= 0.6" | ||
| }, | ||
| "dependencies": { | ||
| "@types/node": "^14.11.2", |
There was a problem hiding this comment.
adding TS things to deps is a very bad idea since the vast majority of consumers are not using TS.
|
Specifically, shipping types with a package is also a very bad idea - types should only EVER be shipped via DT. |
Modules written in TS have always shipped with types, I don't think this is out of the norm. Any particular reason why we shouldn't ship types with cheerio? Regarding the @types/node dependency: Ideally we'd work around pulling this in for all users. One possible solution would be to use |
|
Cheerio isn’t authored in typescript though. One huge problem with it is that you conflate two APIs into the same semver. Needing to make a major bump to cheerio in order to make a breaking change to the types has very bad downstream effects. There’s a reason yargs 15 started shipping types and 16 removed them. |
|
An advantage of shipping the types with the library is it's easier for downstream users to match the types to the version of the library they are actually using. That's been a plus for nock. Breaking changes to types in DT aren't even tracked with semver (i.e. they ship in what would be patch releases) so I'm not convinced DT provides a perfect solution to the question of whether to version the JS API or the types. I'd be interested in the discussions that led to the decision for yargs.
What are the bad effects? |
|
A semver major bump discourages updates, because of the risk of migration effort. TS users have chosen “not JavaScript” and as such, they have signed up for the extra effort that entails. It is unacceptable to burden JavaScript users because some people choose to use an alternative language. |
A changelog helps with this by clarifying what is affected by the semver major.
I'm not sure it's as simple as this. The maintainer's motivation for this PR in #1471 was to improve the VS Code autocompletion for JavaScript users. Maintainers' time should be considered too. As the API changes, maintaining the types within the project is easier than doing it in DT because the changes can be made and tested in the same PR. Shipping the types here is especially important for the contributor workflow for TS users. Developing on or maintaining a fork is much harder when the types are external. I get that there are some negatives to shipping the types within packages, including meaningful negatives for some users who might not ever care about TS, however completely insulating these users from the fact that TS exists seems short-sighted, given the easier maintenance and significant benefits for TS users and many JS users who unknowingly use the TS-driven autocompletion in VS Code. |
|
Wait, if the project isn't authored in TS, how are the types tested in this project? |
|
As in DT, this PR checks in tests. In PRs that change the interface, the type changes are tested too. When a TS project uses Cheerio as a dependency, and modifies the library and its interface, the type changes are necessary for testing the library changes within the context of the project. |
|
I had an conversation offline with @ljharb. I think an accurate summary of the downside of shipping the types here is that it creates an uptick of version bumping. In particular there will be seemingly spurious semver majors which consist only of future breaking changes to the types. If the types were kept in DefinitelyTyped, users who don't care about TypeScript wouldn't have their version numbers affected by these changes. Unfortunately, from the perspective of TypeScript users, DefinitelyTyped does not provide a solution. It doesn't respect semver, and breaking changes to the types get shipped in patch releases (as they were in DefinitelyTyped/DefinitelyTyped#46006). For TS users, it is preferable to have semver for the types. Currently this is only possible if they are included and versioned here, and it's not clear to me whether there even could be a solution that would allow separately tracking semver for the JS and TS interfaces. As a polyglot I'd advocate for what drives all the ecosystems forward together, which is shipping the types here, and to be judicious about breaking changes in the types, just as with breaking changes in the JS interface. |
|
Thanks a lot @paulmelnikow and @ljharb for finding an agreement here and getting to a better solution in the process! Quite a few of the underlying packages for this module already have type definitions. Would it make sense to eg. replace the |
|
@fb55 to be clear, I still don't think types should be shipped in a non-TS package, but it's also not up to me. |
Yup, hope I didn't imply we'd somehow resolved our differences here 😁 |
Hmm, yes, it seems possible that would be better and seems a little odd that wasn't done before. I see cheerio requires htmlparser2 Does cheerio have a way of shipping beta releases? The types I've pulled in here are identical to the last release in DT, though if we're going to make breaking changes to them, it probably would be good to validate them through beta releases. |
The last couple of releases were RCs, which should have been beta versions realistically. I'm okay with making breaking changes to the types before the final
That does make sense. For context: I'd like to transition cheerio to TS eventually, and having the type definitions ready will be helpful. |
Ahhh gotcha! I didn't realize cheerio was already in the midst of 1.0 prereleases. Would you like me to update this to use the upstream types? |
|
I didn't really consider the fact that we have two different parsers in my earlier comments. Let's go with what we have right now, and adjust as we go. Thanks a lot @paulmelnikow for following through here! |
This is an attempt to pull in the Cheerio types from DefintelyTyped so that they're available to all JS users, and so TS + JS users are assured that the type definitions match the version of the library they are using without having to determine the correct version of the @types/cheerio package.
It's the first time I've done this, so feedback from knowledgeable people would be very welcome.
I have two specific questions:
@types/nodeto dependencies because the types useBuffer. Does the library target the browser and React Native? If so will including@types/nodeas a dependency cause problems? Is it preferable to use a reference?declare module 'cheerio'. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/fe3e8016d42f86fff607a7cd424ec3cabeea4b45/types/cheerio/index.d.ts#L304-L307 That wasn't working, so I've changed it here toexport = cheerioModulewhich seems to work. Is there a better way to do that?Before this is merged and shipped we should definitely test that the types can be used successfully by a dependent package. After this is shipped, the @types/cheerio package should be deprecated, and the DT types which depend on Cheerio updated accordingly. I'm not totally sure how to do that, but if someone can point me in the right direction I'm happy to do it.
/cc @matthewmueller @ljharb
Ref #1471