-
Notifications
You must be signed in to change notification settings - Fork 570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
types: Move to DefinitelyTyped? #2261
Comments
I find contributing to Definitely Typed very challenging. I'd much rather us retain management of our types. I've proposed an idea like |
Types on DefinitelyTyped are hard to maintain and only happen as an after-fact. The maintainers of the modules do not have control over the developer experience of their produced, and they are all centralized on a single repository. While the types for Node.js are historically on DefinitelyTyped, more and more that is detrimental for the project and users would be better served if the Node.js team was in charge for maintaining them inside the Node.js tree (so updates in the APIs could be reflected on the types immediately). This issue would be moot if that were the case because the undici sources are already on-tree with the Node.js runtime. This is very hypothetical and not a pragmatic solution, but rather a long-term goal of mine. I think we have a couple of pragmatic options:
Note that undici has a versioning strategy that is slightly different from Node.js, so different Node.js versions have different versions of Undici (and fetch). Having a single set of types across all might work only because fetch is a standard. |
IMO, (3) is not very practical in this case. All the undici types are module files that export both types and values. Because of the way We’re in a bit of a tricky position here—we have a fix ready that involves copying a bunch of your types to DT, and I really want to deliver a fix as soon as possible. But if there’s a chance you want to depend on those types, that’s a better outcome than duplication, so I want to do it in a way that you’re happy with. On the other hand, if you want to publish the types yourself, you’d be doing DefinitelyTyped a huge favor, but it also means we can’t use that as a fix until you do the work of setting up the packaging infrastructure on your end. 😄 I think we’ll want to move ahead with something within ~2 weeks or so. If there’s anything still being sorted out on your end, we’ll probably start by manually copying types into |
-1 for the reasons above and it adds a barrier to contributing, having to make PRs on two different repos when something is changed or added. |
I like options 1 and 2 from @mcollina Furthermore, I had an alternative idea:
|
that has the same contributor barrier that I mentioned above. Copying undici's types seems like a better idea imo |
No just the Fetch types - rest of undici types can live here. Ideally spec-based types will not change frequently at all and could be maintained by other people not just us. |
I think that's a worst of both worlds situation. Being able to only use half of undici with typescript unless you install another package. I'm also wondering what they would do for the custom |
This is potentially something we’d like to do to avoid the existing duplication with the |
@andrewbranch the best outcome really would be to have a generic fetch types that are standalone from the DOM. tbh, we would not have created new types if that was the case. |
I think we would define an undiciFetch type that would augment the standard one. Note that undici is not exposed in Node.js, so the dispatcher is useless there. I think what I understand is that the TS team is going to copy/fork our types into |
If this happens we can't use the types in undici because they'd be incomplete (it was suggested above for undici to rely on the types from the definitelytyped repo). I agree that copying the types is the best solution. |
@mcollina can you clarify exactly how Node.js’s |
|
Thanks for the clarification on
It seems like someone could implement a |
I'd love it if TypeScript could finally publish types for each web api independently. Would help a ton with some WinterCG things too. No reason why the Fetch types should be in DOM. Furthermore, Streams would be great to be independent as well. Here is how we would augment a import type Dispatcher from './dispatcher';
declare module 'fetch' {
interface RequestInit {
dispatcher: Dispatcher;
}
}
export * from '@types/fetch'; It would really be that easy. Afterwards, This may not be perfect but I see this a great path forward. |
And to clarify @andrewbranch -- we only integrate the Its all the other Undici stuff ( |
Having types in a separate package punishes the maintainers and people who want to use typescript with undici. There is no case where I would ever support moving the types to DefinitelyTyped and removing them from undici. |
I don't think we are talking about all of undici's types - just the fetch piece; right? |
Modularizing the web APIs included in TypeScript itself is not happening in the near term, or possibly ever, so I don’t want to get too off-topic on that. There are a lot of really non-obvious challenges blocking that work that are far too in-the-weeds to get into here, so while an ideal solution might be “that easy” to augment from Node/undici’s perspective, it’s nowhere near that easy to set up in practice. At this point, I’m mostly interested in gauging whether there’s interest in undici publishing the existing types as a separate package and on what timeline we might be able to expect to use them if so. Otherwise, or if it seems quite far out, I’ll proceed with copying the types soon. |
Roger Roger! We will not have the types live outside the repo, but I think we can continue a focused discussion on publishing them separately. |
Publishing types as a separate package from within this repo and depending on it from the |
depending on the types in undici is a terrible solution for the reasons I mentioned above. |
@KhafraDev can you restate your reasons? I think we can achieve this within a single repository. In fact, I don't think we would even have to change much. We would only need to add a package.json to We could take this another level and create a monorepo using npm workspaces and changeset. |
It adds a maintenance burden that shouldn't be our responsibility (as long as typescript users don't need to install 2 packages to use undici I don't mind much). Even then, copying the relevant types is a better solution still because they won't need to include the entirety of undici's types, which is more than just fetch. |
I don't see how the maintenance burden will be much different than it already is; we'd just have to do a second npm publish when types change. We could also have @andrewbranch do you foresee copying the fetch only types to be a large maintenance burden? |
@KhafraDev undici+typescript users wouldn't even notice the change; undici would also depend on this hypothetical types package, making it one source of types for everyone. The only extra work is to publish the d.ts with a package.json as a separate npm package and add it as a dependency to undici itself. It could be monorepo, or really just I strongly advice against copying fetch types; if the two diverge it's going to be an annoyance (as is vendoring today). |
I agree with @MKRhere - copying types has a potential to cause worse problems down the line than some minor automation on our part. I've asked previously for a |
There's nothing wrong with vendoring our own types. There is an issue in having to install more packages to get the same functionality (on the flip side: js users shouldn't ever be forced to install an @types package). It shouldn't be up to the undici team to maintain the DefinitelyTyped types, for the same reason that node.js isn't expected to maintain @types/node. Copying the types is the best solution. |
Copying the types will lead to the smallest package size and gives undici maintainers 0 extra work. What is the issue with copying the types..? |
@KhafraDev Maybe there's a misunderstanding. We have moved on from discussing And nobody will need to required to install |
I already did it once and it was about an hour slog for the initial work. Any of the non-global classes needed to be converted to interfaces by hand (mostly just swapping some keywords and removing constructors) so they don’t appear to be real global values once moved into a global declaration space. Imports need to be removed, and then there was sort of a manual tree-shaking process to go down from the globals, keep all the types they reference, and remove everything else. There were enough weird edge cases that this would be hard to automate. At the same time, the public API surface ended up being ~600 lines, so it’s not a small amount to try to manually sync changes between. But assuming the API is quite stable, it might not be a huge deal. My impression of how publishing the types separately would go was that literally nothing would change in your routine maintenance of undici, with the exception of one up-front investment to set up a system of packaging and publishing straight from
I don’t think this was ever on the table. Simply declaring a dependency from the implementation to the types is enough to make this a non-issue.
It would be possible to do this from your own repo too by adding that separation (public API surface vs internal API) into your file structure, but I admit that gets a little messier. I’m personally not at all concerned about |
Adding a dependency to undici does in fact require people to install 2 packages to use undici (technically 4 with busboy which I am ignoring here). Just because it happens automatically does not refute my point.
This is equivalent to managing the definitelytyped types, if this package would be used by @types/node, which I assume it will be because otherwise this separation is pointless. |
We can dual publish:
No more multiple packages (which IMO is not something we should worry about and is a problem for package managers) |
No objections here!
I agree with this. I don’t want to make it seem like I’m demanding you all do free work for DT—that’s why my initial offer was “move the types to DT and you never have to do any work on them ever again if you don’t want to,” but I totally understand wanting to keep doing what you’re doing. Nobody here (except me) is under any obligation to do anything in service of But given the 👍 maybe everyone’s satisfied with publishing a special just-types build for us? 🙂 |
I think so. I would just call it |
Will do 👍 |
We don't have |
Since its just easier to publish all our types I created I'm opening a PR shortly with an automation script. I only published |
Is there a way to tell which version of undici is included in a Node.js binary without looking at the source? It looks like the package.json is removed during the build. |
process.versions.undici |
We now have synchronized undici-types publishing. Feel free to automate around this as you see fit 😄 And let me know if there are any issues. Ready to fix asap 👍 |
Thanks so much @Ethan-Arrowood and everyone! I’ll get started on a DT PR. |
Would it be a big hassle to publish from 5.22 and 5.24? Current @types/node versions include v18.17 and v20.6 which should ideally have those two versions of undici-types, respectively. I’m not sure how significant the changes are and whether a slight mismatch would raise any objections by DT reviewers. Edit for clarity: the major and minor versions of |
Well what if we update Undici in Node.js to the latest 5.25.1 and then we'd all be in sync? Otherwise it'd just take me some time to back port the script, and run it for the 4 or 5 versions |
That would help, especially if you can update in v18.x.x as well as v20.x.x. Then I’d just have to see if there are other changes we need to bring into |
👋 Hello from the TypeScript team.
@types/node
is overdue on including types for the globalfetch
and related classes. (The primary obstacle has been figuring out a method of exposing these globals in a way that doesn’t cause TypeScript errors when DOM typings are also present in the compilation, which include very similar but not identicalfetch
typings. I now have a method that avoids these errors ready to go.)The last question that has to be resolved for
@types/node
is where to get thefetch
types from. Options that have been explored:undici
as a dependency of@types/node
: we decided that this is best avoided sinceundici
is a fairly large package, and it feels like too much weight to include just for a few kB of.d.ts
files..d.ts
files directly into@types/node
: I think this is on the table, but the duplication doesn’t feel great..d.ts
files into a new@types/undici
package that@types/node
can depend on: It seems like your declaration files are hand-written—if we copied them to DefinitelyTyped, you would have the option of deleting them from this repo (though it would not be required—types included in the package itself will have precedence over ones in@types/undici
). We could make any existing undici maintainers owners of the DefinitelyTyped package, which would expedite the review and publishing process when you make changes there. At the same time, the broader DefinitelyTyped community would be able to contribute directly to the maintenance of the types..d.ts
files here but publish them separately: If you’d rather keep the declaration files managed in this repo but are opening to publishing a separate package with just the types, that would work well enough for DefinitelyTyped.We’ll want to pursue one of these options fairly soon, so I wanted to see if any maintainers here had input first. Thanks!
This would solve...
Duplication of
fetch
types between this repo and@types/node
The implementation should look like...
Me adding types to DefinitelyTyped
I have also considered...
Modularizing the DOM
fetch
types to eliminate existing duplication with those types—it’s a possible future enhancement, but a large infrastructural change that’s not necessary to implement now for a good resolution.Additional context
The text was updated successfully, but these errors were encountered: