@thw0rted Thank you for submitting this PR!

🔔 @hertzg @parambirs @tellnes @WilcoBakker @octo-sniffle @smac89 @Flarna @mwiktorczyk @wwwy3y3 @DeividasBakanas @kjin @alvis @OliverJAsh @eps1lon @Hannes-Magnusson-CK @jkomyno @ajafff @hoo29 @n-e @markisme @Taisiias @dex4er @MugeSo @nickmorton @loyd @elithrar @mlamp @TitaneBoy @orblazer - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

@thw0rted The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Whoever comes to review this, can you take a look at that Travis build? It looks like some kind of internal error, not related to the PR.

Node part looks correct.
@thw0rted regarding CI fail I have no idea... maybe retry by pushing again or close/reopen.

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

Hey @thw0rted @Flarna just so you're aware, this type of change breaks compatibility with older node type definitions. Because you updated @types/node for version 10 with a new interface, using that interface in any other package (like @types/ws) means they now have a dependency on the version 10 node types. The version of the @types package is in general supposed to match the version of the npm package it describes like that so you can easily tell what version of the package the types describe (since there are typically breaking interface changes between major versions).

This creates a breaking situation for anyone using the ws package and the associated types on a version of node < 10, because they will want to use an @types/node package that accurately describes their node version (e.g.: @types/node@6 if they're using node 6 LTS) but they don't have this new interface.

I have opened a PR to fix this issue #26196 in @types/ws, but I didn't realize this was a change made to @types/node at the same time, so I would also recommend reverting the other changes here that affect swaggerize-express, smtp-server, and net-keepalive as I imagine they don't actually require node >= 10 either.

I thought the way NPM was supposed to handle this was that the top level project can rely on @types/node@6 and @types/ws@latest, and NPM would automatically (?) add a second-order dependency under node_modules/@types/ws/node_modules/@types/node with the v10 typings. Is that not what's happening? Is it not desirable?

Alternately: in this specific case, I believe the problem is that an anonymous interface was given a name and exported, then used in the packages you listed. The interface has existed and been used in more or less all versions of @types/node... would it be easier to just to back through all the older versions and make the same changes I made in v10 (and I think maybe also v9)?

NPM does not resolve it that way in this case, no. The dependencies for @types/ws lists "@types/node": "*" which means any version satisfies the constraint, and it will install the latest if none is already installed. I think it would also be undesirable for it to install its own dependency for node 10 just for this interface, as it doesn't actually seem to have a lot to do with changes made in node 10.

You could also go back and make the interface available in older versions of node, but to do it the right way you'd want to find out when that interface was introduced and go all the way back to that version. After reading through your changes and the ws code that actually returns that AddressInfo object a little more, I think that might actually be most appropriate since it is returning the object directly from http.Server's address() method.

I'm still kind of new at this, but I think the problem is one of conventions in the vanilla-JS world. Before TS came along, JS didn't have the vocabulary to discuss interfaces -- at least, not something universal and consistent. So you had years of docs saying "this function returns an object with the following structure..." but never gave a name to that structure. Since the canonical docs that were (probably) used to generate the initial DT typings don't explicitly label an interface (and why would they?), it's up to individual DT authors to decide if and how to name interfaces -- and once that decision is made, it has ramifications for all versions in the future.

Now, with the benefit of hindsight (!), I can say that the decision to not name the interface used in these functions -- or worse yet, to name it in some places and not others -- was a mistake. But retroactively fixing that mistake, going to a better design, causes the breakage you're seeing. Like I said, I'm new here, so I don't know how the community wants to see these problems solved. I think the structure-I-call-AddressInfo has been in Node for as far back as I can still find docs, but whose code am I going to break if I reach back into older versions and label it?

No worries, I can't really say I'm an expert either but I've got some experience under my belt with tracking down these type definition issues since we deal with quite a lot of them at my work.

Regarding your concerns about labeling the interface, it's not really the giving it a name that breaks it, it's that you added that name in a version of the node types that not everyone can use, and then imported it into the dependent packages. The name itself in TS isn't taken into account when it comes to type compatibility. Your AddressInfo and someone else's anonymous { port: number; family: string; address: string; } are perfectly compatible, you can replace one with the other just fine. Here's some more info on the subject: https://www.typescriptlang.org/docs/handbook/type-compatibility.html

I'm quite sure that net.server.address() returns string | AddressInfo since more or less always. The string case is if a Pipe/UnixDomainSocket is used, AddressInfo in case a TCP socket is used. The string case was not documented from node for a long time (see nodejs/node#12907) so searching through the history of node docs will not give the truth.
Dependencies between typings like this are a pain as on the one hand it's perfectly valid to mix latest ws with node 6, 8 or 10 - but most fixes for node typings are only done in latest version.

In this case it would be most likely best to update also older node versions. But as this change is breaking for quite some users - pipe/domain socket servers are use rare and even then users know what they use but after the change a cast/type check is needed it may result in complains.
I was thinking about using a generic to select between TCP/Pipe but as it is possible to switch between them it's not possible.