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

In package.json, move @types/* from dependencies to devDependencies #673

Open
MuleaneEve opened this issue Apr 8, 2023 · 9 comments
Open
Labels
question Further information is requested

Comments

@MuleaneEve
Copy link

Describe the bug
Currently, engine.io has @types/cookie, @types/cors and @types/node as dependencies. They should instead be devDependencies.

engine.io/package.json

Lines 33 to 36 in 7033c0e

"dependencies": {
"@types/cookie": "^0.4.1",
"@types/cors": "^2.8.12",
"@types/node": ">=10.0.0",

This is causing my projects to take many more packages as dependencies at runtime.

@MuleaneEve MuleaneEve added the bug Something isn't working label Apr 8, 2023
@darrachequesne
Copy link
Member

Hi! Please see the rationale there: microsoft/types-publisher#81 (comment)

@darrachequesne darrachequesne added question Further information is requested and removed bug Something isn't working labels Apr 10, 2023
@MuleaneEve
Copy link
Author

My understanding of that comment is that, for @types\x packages, it is ok to put their own (dev)dependencies in dependencies.
I think that's fine because the end-user of these @types\x packages will import them as devDependencies. Therefore, everything that comes along with these packages will also be treated as devDependencies.

As a practical example: The package sharp has a devDependency on @types/node (source code).
So, its @types\sharp includes @types/node as a "regular" dependency (source code).

However, engine.io cannot be imported just as devDependencies. Therefore, all its dependencies become "real" dependencies.

@MuleaneEve
Copy link
Author

@darrachequesne Can you please revisit this issue?

@darrachequesne
Copy link
Member

Hmm, that's not my understanding:

The short answer would be for module authors - if, as an author, you want to publish your TypeScript package to NPM and the dependencies are in the development section, no one will be able to install it and use it without having to manually install your types dev dependencies.

This was discussed here for the v3 release: socketio/socket.io#3690

See also: https://stackoverflow.com/a/46011417/5138796

@MuleaneEve
Copy link
Author

MuleaneEve commented Apr 26, 2023

I see that this topic has been discussed multiple times. Sorry if I still bring it up.

After reading both links, I still don't understand why moving @types/* to devDependencies would prevent end-user projects from compiling: The types would still be available for compilation since dev packages are installed at that point.

Maybe I am missing something...

By the way, when v3 was released, I assume that many other users did succeed in using it (both in dev and in production). So I wonder if the author of that issue had another weird problem that was worked-around by moving the dependencies...

Edit: Also, I have never seen any other npm package that needed to do this. So, I wonder what is unique here.

@darrachequesne
Copy link
Member

That's because those types are exposed in the public API of the engine.io package. If you check the build/server.d.ts file in the published package, you will see:

/// <reference types="node" />
import { EventEmitter } from "events";
import { IncomingMessage, Server as HttpServer } from "http";
import { CookieSerializeOptions } from "cookie";
import { CorsOptions } from "cors";

// ...

export interface ServerOptions {
    cookie?: (CookieSerializeOptions & {
        name: string;
    }) | boolean;

    cors?: CorsOptions;
}

So they need to be installed alongside the engine.io package, hence the dependencies. If they were listed as devDependencies instead, the user would need to manually install them:

$ npm install engine.io @types/cookie @types/cors @types/node 

@MuleaneEve
Copy link
Author

build/server.d.ts is only relevant when developing and compiling a project. And in these contexts, the devDependencies should be installed. So, if those types are declared as devDependencies by the engine.io package, everything should continue to work.

@MuleaneEve
Copy link
Author

MuleaneEve commented Apr 26, 2023

@MuleaneEve
Copy link
Author

@darrachequesne I finally figured out what I was missing: devDependencies are not recursively installed like dependencies. Somehow, I assumed that was the case.

So if we still want to resolve this issue, the only idea I have is to clone CookieSerializeOptions and CorsOptions into engine.io. Then, those types packages will not be needed. But I understand that it would be annoying to keep them in-sync (though, these interfaces change very rarely; see 1 & 2).

What do you think?

darrachequesne added a commit that referenced this issue May 1, 2023
In order to prevent issues like:

> error TS2345: Argument of type 'RequestHandler<ParamsDictionary, any, any, ParsedQs, Record<string, any>>' is not assignable to parameter of type 'Middleware'.
>  Types of parameters 'req' and 'req' are incompatible.
>  Type 'IncomingMessage' is missing the following properties from type 'Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>': get, header, accepts, acceptsCharsets, and 29 more.
>
>  io.engine.use(sessionMiddleware);
                 ~~~~~~~~~~~~~~~~~

Related: socketio/socket.io#4644

We could also have use the RequestHandler type from the
@types/express-serve-static-core package, but that would add 5 new
dependencies.

See also: #673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants