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

include "typings" field in package.json #2818

Open
rolandjitsu opened this issue Nov 30, 2015 · 30 comments
Open

include "typings" field in package.json #2818

rolandjitsu opened this issue Nov 30, 2015 · 30 comments

Comments

@rolandjitsu
Copy link

Now that the TS compiler supports module resolution via node, it would be nice not to use TSD to install the typing but instead have them included with the npm package.

@dougwilson
Copy link
Contributor

We don't have any typings. What would we put in the field?

@blakeembrey
Copy link
Member

@dougwilson I would be interested in maintaining them, if that sounds reasonable.

Edit: I'll be working on some related type testing tools in https://github.com/typings/typings which will complement any such support for typings.

@dougwilson
Copy link
Contributor

@blakeembrey , that is absolutely reasonable! I was just talking about these typings today and I think Express should provide official typings for TypeScript as a feature! This means that we would be invested in properly maintaining them as part of the normal commit and release process. If you want to provide the initial typings for Express, that would be amazing!

Ideally I would love it if we could achieve these things:

  1. Provide bindings to users of Express
  2. Add a step to our CI that would validate that the bindings are correct somehow (to help prevent them from deteriorating accidentally)

These can be independent tasks, but I think Express is going to say yes to supporting TypeScript binding out of the box.

@blakeembrey
Copy link
Member

Fantastic, I'll work on making sure the CI step is solved properly this week.

@rolandjitsu
Copy link
Author

@blakeembrey that sounds great 👍 @dougwilson does this mean that typings will also be added to the other expressjs packages (morgan, session, etc.)?

@dougwilson
Copy link
Contributor

does this mean that typings will also be added to the other expressjs packages (morgan, session, etc.)?

yes

@rolandjitsu
Copy link
Author

Great, because I was having a hard time figuring out how to write those typings properly as I kept getting some errors about the way I was declaring them (I was using ambient typings and I realized that is not the way to go).

@LinusU
Copy link
Member

LinusU commented Feb 15, 2016

We actually had this problem come up in multer where the type file that someone used wouldn't account for our middleware adding fields to the req object (namely req.file). Is this a problem that could be solved somehow? I think that it affects most middleware...

Other than that, we would be happy to include and maintain "typings" for multer as well :)

@blakeembrey
Copy link
Member

@LinusU Mutations are hard to represent in TypeScript, especially with things like adding a field during middleware execution. It's possible to merge the interfaces and we should do that, if nothing else, but I'm not aware of a way to declaratively merge types depending on possible in middleware. That's something I've been thinking about anyway, but I'm glad to share an example of how this would work.

The reason I haven't yet tried to merge this into Express is because I haven't had time to solve the test suite involved. The closest I have is a feature I've had for a while - bundle the typings, then run the TypeScript compiler, then run unit tests as usual.

@jeffbcross
Copy link

Would maintainers be interested in converting the express source to TypeScript, so we could automatically generate correct typings with each release? I'd be down for helping with the conversion. // @dougwilson @ritch

@dougwilson
Copy link
Contributor

Hi @jeffbcross, thanks for the offer, but there is no plan to convert the source to anything other than pure JavaScript, I'm sorry.

@jeffbcross
Copy link

Thanks @dougwilson I'm asking about openness to such a thing. I.e. knowing your contributor base, would typescript support be something contributors would want (or at least live with)? I could enumerate the benefits here beyond typings generation if it would be helpful.

@dougwilson
Copy link
Contributor

Hi @jeffbcross, I am generally aware of the benefits, but I don't think it aligns with our user base, or even our maintainers. For example, I don't even understand TS and really can't even judge it unless there is a class I can take to learn it? The biggest barrier is that unless the idea is that we transfer ownership of express to you, at the very least the current contributors should know TS well enough to continue to contribute to this module.

I know there is a very minor community that would like TS bindings, mainly for the IDEs, but other than that, I have no real idea how it would impact the user base.

Thoughts? And why would it be necessary to completely change the language this module is written in just to declare type bindings?

@dougwilson dougwilson added 4.x and removed 5.x labels Aug 5, 2017
@dougwilson dougwilson added this to the 4.16 milestone Aug 5, 2017
@dougwilson dougwilson removed this from the 4.16 milestone Sep 28, 2017
@wesleytodd
Copy link
Member

I was looking for issues related to typescript and stumbled upon this. Can we close this since it has been inactive for so long? Or maybe @blakeembrey can close it along with a status of the typescript support for express?

@dougwilson
Copy link
Contributor

It still seems like a good thing to have, even though there hasn't been any movement. I guess the question is do we want this at some point? If so, we don't have a good way to track this wait besides having an open issue somewhere about it, right?

@wesleytodd
Copy link
Member

I guess I assumed that we were good with just pointing people to the DT repo. I think it is surface area that express should probably avoid supporting since only Blake is familiar with the requirements, and the DT project seems to have the support behind it.

@dougwilson
Copy link
Contributor

Gotcha. I never got into TS myself, but I see the value of it. I'm OK with adding typings if that helps the TS community in some way, though I just don't know enough to really help a ton. I guess we can close this and reopen (or a new one opened) if there is a proposal, ideally including the actual definitions, to add into Express.

@simonmeusel
Copy link

What about the types from here: https://www.npmjs.com/package/@types/express

@wesleytodd
Copy link
Member

That is the package we were talking about above :)

@simonmeusel
Copy link

ok sry

@wesleytodd
Copy link
Member

In #3972 @commonbreed mentions those typings are out of date. Can you provide some more details here? What is out of date, is there a simple PR we can make to update them?

@craben20
Copy link

craben20 commented May 31, 2019

@wesleytodd It's likely that the "outdated" types are still accurate, as I don't know whether any signatures have changed from 4.16.1 to 4.17.1. Regardless, it would be nice to have the types package consistent with Express versioning. (Every Express release, make a PR to the types package?)

@craben20
Copy link

Although I think a TS port is more appropriate.

@wesleytodd
Copy link
Member

Thanks @commonbreed. Can you explain what you mean by "TS port"?

It's likely that the "outdated" types are still accurate

whether any signatures have changed from 4.16.1 to 4.17.1

I see what you mean, and the answer is probably that they are still accurate.

it would be nice to have the types package consistent with Express versioning

If this is something the typescript community is used to, then we would probably like to support that. That being said, we would need someone from the community who actively uses typescript to step up and maintain them. Is that something you would be interested in?

@craben20
Copy link

Ah, just noticed dougwilson has prohibited porting this code base to TypeScript, which is reasonable in the short term - seems to be what many middlewares are opting for too.

If this is something the typescript community is used to, then we would probably like to support that.

Unfortunately, this is not something the community is used to as such. Types don't tend to be updated until there is a significant code refactor, or until someone reports them as broken. It can be confusing to TypeScript developers when they see npm has pulled an "older" types package. I think it's probably very safe to add DT typing as a dependency (not a devDependency) - this is a library. I'll make a PR now.

@craben20
Copy link

craben20 commented May 31, 2019

#3973 Here's the PR

@wesleytodd
Copy link
Member

dougwilson has prohibited porting this code base to TypeScript

Doug is not a dictator. He and the rest of the contributors do not want to port anything to TS. I believe if you search the repo you can find many conversations where we have all shared our opinions on this, so I will not repeat them. But I did not want someone to come along and read this and miss-understand the situation by reading your comment alone.

Unfortunately, this is not something the community is used to as such.

Ok, I don't won't to add more on that PR discussion yet. But from your comment here it sounds like we do not want to do something which is not already in line with what the TS community expects. If they already are used to installing @types/express and the version miss-match is fine as long as the types are still correct, then I would prefer to leave it that way.

I think it's probably very safe to add DT typing as a dependency

Again, not getting into the issues in the PR, I disagree that adding DT as a dep here is a good thing. It is not a dep, and since users can opt into typescript and other not, we don't want to make changes which effect everyone for the sake of the few.

@wesleytodd wesleytodd mentioned this issue Jun 3, 2019
39 tasks
@craben20
Copy link

craben20 commented Jun 7, 2019

Doug is not a dictator. ...

I'm fully aware this is a community project and never supposed Doug to be such. Perhaps "prohibited" was a little strong, but the sentiment was to suggest that a TypeScript port is likely not going to be on the table any time soon, so to consider alternative options.

If they already are used to installing @types/express and the version miss-match is fine as long as the types are still correct, then I would prefer to leave it that way.

My reason for picking up this issue in the first place was to address specifically Express' lack of typings. Express has over 9M weekly downloads on NPM, and has a large community of middleware developers who might depend on solid, directly maintained types for linting, integration into CI/testing etc.

It is not a dep

I mostly concur. Adding this dependency here is probably not the right move. It causes all kinds of issues with versioning and redundancy. I would however urge folks to consider ensuring PRs are at least made to DT's Express types to ensure any API changes are immediately reflected for TS developers. Don't need to make them a dependency - but at least ensure they're up to date. TypeScript is involved in over 30% of projects on NPM, so it's important for types not to get left behind.

@wesleytodd
Copy link
Member

I'm fully aware this is a community project and never supposed Doug

I was not admonishing you, just making sure the governance model was clear to everyone who might read this in the future :)

I would however urge folks to consider ensuring PRs are at least made to DT's Express types to ensure any API changes are immediately reflected for TS developers

I fully agree here. We would love to have someone who has the right knowledge in this area to participate and ensure that we have updated typings. If that person is you, then I would suggest following the repo so you see when update branches are being prepared. Today the matter is just that none of the active core team members (except @blakeembrey who is busy with many other things) are TS users. So we just don't have the person power to make it happen.

@craigcosmo

This comment has been minimized.

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

No branches or pull requests

9 participants