Skip to content
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

Closed
andrewbranch opened this issue Sep 11, 2023 · 46 comments
Closed

types: Move to DefinitelyTyped? #2261

andrewbranch opened this issue Sep 11, 2023 · 46 comments
Labels
enhancement New feature or request Types Changes related to the TypeScript definitions

Comments

@andrewbranch
Copy link

👋 Hello from the TypeScript team. @types/node is overdue on including types for the global fetch 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 identical fetch 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 the fetch types from. Options that have been explored:

  • Add undici as a dependency of @types/node: we decided that this is best avoided since undici is a fairly large package, and it feels like too much weight to include just for a few kB of .d.ts files.
  • Copy much of undici’s .d.ts files directly into @types/node: I think this is on the table, but the duplication doesn’t feel great.
  • Move undici’s public API .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.
  • Keep the .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

@andrewbranch andrewbranch added the enhancement New feature or request label Sep 11, 2023
@Ethan-Arrowood
Copy link
Collaborator

I find contributing to Definitely Typed very challenging. I'd much rather us retain management of our types. I've proposed an idea like Keep the .d.ts files here but publish them separately in the past; I don't think we had a strong reason for no. Moreover, we've always had a strong preference for shipping types alongside the module itself. I'm open to new possibilities though 👍

@mcollina
Copy link
Member

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:

  1. move the fetch types to DefinitelyTyped and import them from there. I don't expect the the fetch() types to change much over time, so it might work for us.
  2. publish a module from this repository called @nodejs/fetch-types or similar, and have @types/node depend on it
  3. having some automated script that push changes from these repos to DefinitelyTyped @types/node

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.

@andrewbranch
Copy link
Author

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 @types packages and TS work together, porting them as-written into @types/node would incorrectly signal that those files can actually be imported from e.g. "node/undici/request.js", which is of course nonsense. The amount of massaging it takes to get the contents of those files into @types/node while being externally invisible would be a lot more work to automate than I think it’s worth.

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 @types/node directly, and switch to whatever longer term method of sharing you decide on when you’re ready. If you have your own types package or decide to move to @types/undici before that point, great! I’ll be happy to help however I can.

@KhafraDev
Copy link
Member

-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.

@Ethan-Arrowood
Copy link
Collaborator

I like options 1 and 2 from @mcollina

Furthermore, I had an alternative idea:

  • Create a @types/fetch that is Fetch types to-spec.
  • We depend on @types/fetch and utilize declaration merging in order to change the types to our non-spec ways (if there are any).

@KhafraDev
Copy link
Member

that has the same contributor barrier that I mentioned above. Copying undici's types seems like a better idea imo

@Ethan-Arrowood
Copy link
Collaborator

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.

@KhafraDev
Copy link
Member

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 dispatcher option - copy a majority of the types to support it? At that rate they might as well just stay in undici.

@andrewbranch
Copy link
Author

Create a @types/fetch that is Fetch types to-spec.

This is potentially something we’d like to do to avoid the existing duplication with the fetch that’s included in TypeScript’s lib.dom.d.ts. However, there are some unanswered questions as to how that should work. (Would TypeScript take a dependency, or change something about its included lib files? The reason this doesn’t already work today is that you can’t access the included fetch types without polluting the global namespace with all the DOM types.) At any rate, if someone does champion that effort, I think it could be integrated into any of the other approaches we’ve discussed so far after the fact.

@mcollina
Copy link
Member

@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.

@mcollina
Copy link
Member

I'm also wondering what they would do for the custom dispatcher option - copy a majority of the types to support it?

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 @types/node if we don't find a way to solve this conundrum.
This is a good option anyway, but it would require some work to keep it going and avoid divergence long term.. but given that's spec driven, I'm not really too concerned.

@KhafraDev
Copy link
Member

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.

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.

@andrewbranch
Copy link
Author

Note that undici is not exposed in Node.js, so the dispatcher is useless there.

@mcollina can you clarify exactly how Node.js’s fetch API differs from undici’s? I was under the impression that Node.js simply exposed undici’s public API exports of fetch, Request, Response, Headers, and File on globalThis.

@KhafraDev
Copy link
Member

KhafraDev commented Sep 12, 2023

File is part of both undici and node. undici's came first but was eventually added to node; node doesn't expose undici's, they're separate. For fetch we added a custom dispatcher option, an example of which is shown https://github.com/nodejs/undici#undicifetchinput-init-promise. FormData is also exposed from undici.

@andrewbranch
Copy link
Author

Thanks for the clarification on File, and I forgot about FormData but have it in my WIP PR. For dispatcher, I don’t understand why that would not be part of the Node.js fetch API:

> fetch.toString()
'async function fetch(input, init = undefined) {\n' +
  '    return lazyUndici().fetch(input, init);\n' +
  '  }'
>

It seems like someone could implement a Dispatcher given the interface, even if Agent is not directly accessible from Node.js.

@Ethan-Arrowood
Copy link
Collaborator

Ethan-Arrowood commented Sep 12, 2023

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 @types/fetch for Undici:

import type Dispatcher from './dispatcher';

declare module 'fetch' {
  interface RequestInit {
    dispatcher: Dispatcher;
  }
}

export * from '@types/fetch';

It would really be that easy.

Afterwards, @types/node (which I hope will eventually not be in DefTypes but inside of Node itself someday), can depend on @nodejs/undici-types if it must in order to re-export the augmented Fetch types.

This may not be perfect but I see this a great path forward.

@Ethan-Arrowood
Copy link
Collaborator

And to clarify @andrewbranch -- we only integrate the fetch piece from undici into Node.js. It is the one-and-the-same implementation you see here that is available as a global in Node 18+.

Its all the other Undici stuff (request, stream, Dispatcher, etc.) that are not.

@KhafraDev
Copy link
Member

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.

@Ethan-Arrowood
Copy link
Collaborator

I don't think we are talking about all of undici's types - just the fetch piece; right?

@andrewbranch
Copy link
Author

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.

@Ethan-Arrowood
Copy link
Collaborator

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.

@metcoder95 metcoder95 added the Types Changes related to the TypeScript definitions label Sep 13, 2023
@MKRhere
Copy link

MKRhere commented Sep 13, 2023

Publishing types as a separate package from within this repo and depending on it from the undici and @types/node packages seems like the best scenario by a long shot. And then we can all stop vendoring undici types (telegraf); It'll be great! I'm excited that DefinitelyTyped/DefinitelyTyped#60924 will finally be resolved.

@KhafraDev
Copy link
Member

depending on the types in undici is a terrible solution for the reasons I mentioned above.

@Ethan-Arrowood
Copy link
Collaborator

@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 types/ and update our release process.

We could take this another level and create a monorepo using npm workspaces and changeset.

@KhafraDev
Copy link
Member

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.

@Ethan-Arrowood
Copy link
Collaborator

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 undici depend on its own types package and re-export them so that TypeScript users only have to install undici to get both the API and types automatically. This is very automatable using monorepo tooling.

@andrewbranch do you foresee copying the fetch only types to be a large maintenance burden?

@MKRhere
Copy link

MKRhere commented Sep 13, 2023

@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 a script that quickly creates a package.json in a temporary folder, copies index.d.ts to it, and publishes it a folder with a package.json and index.d.ts. Small task for undici, but unblocks everyone.

I strongly advice against copying fetch types; if the two diverge it's going to be an annoyance (as is vendoring today).

@Ethan-Arrowood
Copy link
Collaborator

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 @nodejs scope (nodejs/admin#794) and ideally we could use that. We could Bikeshed this further and consider a @undici scope.

@KhafraDev
Copy link
Member

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.

@KhafraDev
Copy link
Member

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..?

@MKRhere
Copy link

MKRhere commented Sep 13, 2023

@KhafraDev Maybe there's a misunderstanding.

We have moved on from discussing @types/undici managed by community or nodejs/undici managing a DefinitelyTyped package, and are discussing @nodejs/undici published from and managed by this repository.

And nobody will need to required to install @nodejs/undici separately. undici would itself will depend on @nodejs/undici, so when you install undici you automatically receive its type definitions just as before.

@andrewbranch
Copy link
Author

andrewbranch commented Sep 13, 2023

do you foresee copying the fetch only types to be a large maintenance burden?

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 ./types. Making a monorepo/changesets/workspaces setup might eliminate some custom work from that setup at the cost of being a more visible change to your repo structure.

as long as typescript users don't need to install 2 packages to use undici

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.

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.

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 @types/node having a dependency on a few more .d.ts files than it actually needs.

@KhafraDev
Copy link
Member

KhafraDev commented Sep 13, 2023

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.

and are discussing @nodejs/undici published from and managed by this repository.

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.

@Ethan-Arrowood
Copy link
Collaborator

We can dual publish:

  • @nodejs/undici publishes all of undici source and type files (from the types directory)
  • @nodejs/undici-types is a secondary publish of just the types directory

No more multiple packages (which IMO is not something we should worry about and is a problem for package managers)

@andrewbranch
Copy link
Author

We can dual publish

No objections here!

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.

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 @types/node. DefinitelyTyped exists to serve the TypeScript (and JavaScript, via any editor using TS Server!) community; the TypeScript team and a ton of dedicated DT volunteers are extremely invested in maintaining it, and its usual purpose is filling in for libraries where the author didn’t care about TypeScript at all. I think we have a different (better) relationship with Node.js, since I think pretty much everyone recognizes the benefits of having a high-quality and up-to-date @types/node available. But if the verdict is that it’s best for DT to do its own thing without involving undici, that’s totally fine—it’s the norm for DT maintenance and we can absolutely make it work.

But given the 👍 maybe everyone’s satisfied with publishing a special just-types build for us? 🙂

@mcollina
Copy link
Member

But given the 👍 maybe everyone’s satisfied with publishing a special just-types build for us? 🙂

I think so. I would just call it undici-fetch-types or @nodejs/undici-fetch-types. @Ethan-Arrowood could you get that started?

@Ethan-Arrowood
Copy link
Collaborator

Will do 👍

@Ethan-Arrowood
Copy link
Collaborator

We don't have @nodejs yet so I'm going to publish to just undici-fetch-types to start. We can always change that later

@Ethan-Arrowood
Copy link
Collaborator

Since its just easier to publish all our types I created undici-types instead: https://www.npmjs.com/package/undici-types

I'm opening a PR shortly with an automation script. I only published 0.0.1 to start in case we need to make any changes. The script will enable us to publish types matching the version of undici itself

@andrewbranch
Copy link
Author

andrewbranch commented Sep 19, 2023

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.

@KhafraDev
Copy link
Member

process.versions.undici

@Ethan-Arrowood
Copy link
Collaborator

@andrewbranch #2273 (comment)

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 👍

@andrewbranch
Copy link
Author

Thanks so much @Ethan-Arrowood and everyone! I’ll get started on a DT PR.

@andrewbranch
Copy link
Author

andrewbranch commented Sep 20, 2023

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 @types/node are supposed to correspond to the same major/minor of Node.js, while patch versions of DT packages are a simple counter for bug fixes. I plan to set the dependency on undici-types with a ~ version range, so a user who installs @types/node may get (e.g.) undici-types@5.25.99 even if the version of undici bundled in their Node.js is 5.25.1.

@Ethan-Arrowood
Copy link
Collaborator

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

@andrewbranch
Copy link
Author

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 @types/node@20.7.x since it’s currently a minor version behind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Types Changes related to the TypeScript definitions
Projects
None yet
Development

No branches or pull requests

6 participants