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/mysql should be in devDepenencies #72

Closed
gamliela opened this issue Nov 13, 2017 · 9 comments
Closed

@types/mysql should be in devDepenencies #72

gamliela opened this issue Nov 13, 2017 · 9 comments

Comments

@gamliela
Copy link

@types inclusion should be optional.
Use case: in our project we share code for client and server, and server types conflict with client types. Consider setInterval, which has different return type on Node and DOM.

Also, I think that this is the convention for many libraries, since TypeScript in general is optional.

@CodeFoodPixels
Copy link
Owner

CodeFoodPixels commented Nov 13, 2017

microsoft/types-publisher#81 states that they should be installed as a dependency so that it's installed without people having to mess around. To me this makes sense.

I don't see how mysql types will have any effect on your frontend types.

/cc @RetroCraft @move-zig because they added/changed the TypeScript stuff.

@move-zig
Copy link
Collaborator

There was a bit of a discussion about this on https://gitter.im/DefinitelyTyped/DefinitelyTyped from Nov 06 19:47 to Nov 07 08:23 (ET, I think). It seems like the consensus was a standard dependency, not devDependency. I think that makes sense.

@RetroCraft
Copy link
Contributor

The idea is that @types dependencies should be installed when you install the package. When it was setup as just a devDep as before, you don't know you need to install it until you try and run/build/lint/whatever and it errors out saying that it can't find a definition for the mysql module.

@gamliela
Copy link
Author

I understand the rationale for this change. Still, there is a reason why many libraries put it in devDependencies after all, and some of these reasons are given in the links you presented.

Here are my arguments:

  1. TypeScript is not mandatory. By putting types in the dependencies section you signal that the types are essential and required part of the project, which is not the case.

  2. It is easy to add @types for those who want it, but it is much harder to remove it for those who don't want it (like me). For me, it involves to write a specific postinstall script to deal with this problem.

  3. There is a technical problem that affect (probably minority) of users. Some types are inferred incorrectly. See discussion at the end of this thread, and note that eventually they chose to put @types/node at devDependencies.

I will try to explain # 3 from our perspective. A variation of our problem is demonstrated also here.

We have sever code (Node) and client code (ES5) on the same project tree. Both share the same tsconfig file and both code parts get compiled together. Let's put aside the question whether it's a good setup or not; that's the way we work now and I assume we are not the only ones that work this way.
In our client code we have expressions like setTimeout(...). We actually mean window.setTimeout(...) (doc) and therefore expect to get a number type as result. But because of the inclusion of @types/node (as side effect of @types/mysql inclusion) the TypeScript compiler automatically picks the node version of setTimeout, which returns a Timer object instead of number. So we ends up with TS2323: Type 'Timer' is not assignable to type 'number' error.

@CodeFoodPixels
Copy link
Owner

  1. Typescript isn't mandatory and unless you actually try and use typescript, it has no real effect. Someone can still use ES5/ES6.
  2. As @types/mysql is a dependency of the types in this library, they absolutely should live in dependencies. It defeats the point of having a dependency when users then have to install a separate package to make yours work, especially when it's not a dependency of their project, but a dependency of mine.
  3. As mentioned, this is a minority of users. The dependencies they moved were dev-only dependencies, as can be seen in the changelog file in this commit: Polymer/dom5@634d5bb

Let's put aside the question whether it's a good setup or not

That's pretty key to this issue actually. If my library didn't uncover this problem, another one would have. The fact your workflow is broken because of a bad setup should have no bearing on the correct implementation of my node library.

Also, because there's an xkcd for everything:

@gamliela
Copy link
Author

gamliela commented Nov 14, 2017

I never said our workflow is broken. It is, I agree, questionable. It's way too opinionated to disallow it completely.
I also didn't claim that types are not dependencies. They are indeed. But there is a vague line between dependencies and devDependencies, and since types information are definitely not runtime dependencies one can argue that they are devDependencies.

@CodeFoodPixels
Copy link
Owner

CodeFoodPixels commented Nov 14, 2017 via email

@RetroCraft
Copy link
Contributor

@gamliela Is there any reason why you can't write window.setTimeout? If you have Node code you should be including @types/node anyways, yes?

@gamliela
Copy link
Author

@RetroCraft No specific reason. It's just a lot of places to change, and also the problem occurs in other variations (e.g. require function). And yes, we do have Node code, but since it sits together with non-Node code we are very picky about the types we throw in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants