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

Add DT typing support #3973

Closed
wants to merge 2 commits into from
Closed

Add DT typing support #3973

wants to merge 2 commits into from

Conversation

craben20
Copy link

Typing for Express should be granted by the DefinitelyTyped package, if not locally maintained.

@dougwilson
Copy link
Contributor

Should the version of the DT package match the express version? Just based on that version number, it would seem like it is missing the new apis in 4.17?

@craben20
Copy link
Author

I will make a PR to @types/express (and @types/express-serve-static-core) to get any API changes added in when I get back home late tonight (UK time). NPM will track the minor version. If you'd like to wait to then before you consider committing, that's fine. Being dependent on the project will undoubtedly spur on maintenance, and adding a couple signatures here and there for API changes is no great challenge (but people need to use it)

@dougwilson
Copy link
Contributor

Well, this PR will lock the types to the 4.16 ones, and if we make a release, then I guess we'll have to make another release of express again to pick up the 4.17 types?

Also I see that types package has a bunch of dependencies, all set to "". For example it brings in "" of body parser, but Express here will only have 1.x of body parser, so once there is a 2.x of body parser, the express installed types will be invalid. Can you get the dependency versions of the type package locked in to the matching versions included with express?

@craben20
Copy link
Author

craben20 commented May 31, 2019

import * as bodyParser from "body-parser" is the TypeScript equivalent of const bodyParser = require("body-parser"), so doesn't have the behaviour you describe. The version of body-parser used will be whichever is installed by the application (so basically whichever version Express depends on).

I have also corrected the semantic versioning error.

@craben20
Copy link
Author

The failed test appears to be unrelated.

@dougwilson dougwilson closed this May 31, 2019
@craben20
Copy link
Author

@dougwilson Why close this PR?

@dougwilson
Copy link
Contributor

I closed the PR because I think there needs to be an actual discussion on how this is going to work, because how this is implemented here is going to just be a worse experience for users. For example, if you set the dependency to ^4.16.1, then when someone installs express 4.17.3, they can end up with the types for Express 4.23.0, which would be incorrect for that version of Express.

In addition, the body-parser example you can see in the @types/express package. That package is just not going to be possible to have as a dependency due to this:

$ npm info @types/express dependencies
 
{ '@types/body-parser': '*',
  '@types/express-serve-static-core': '*',
  '@types/serve-static': '*' }

As you can see there, it will pull in any version of body-parser, which means that when Express 5 comes out (the coming month) along with body-parser 2, then suddenly new Express 4 install types will be completely broken, since they will pull in the 2.x version of body-parser instead of 1.x.

The management of those dependencies and their ranges needs to be locked into the same ranges that match those actually shipped with the installed version of express so the typings are relevant to the actual exposed APIs of the installed express version.

@craben20
Copy link
Author

craben20 commented May 31, 2019

DefinitelyTyped's types are best effort, and there is an ongoing debate at Definitely Not Versioned. Generally, there's no harm in including whichever version of the types is available. It just makes development easier for us TS guys.

Also, I'm not sure what you're showing me in that last comment, because I see dependencies on other @types packages, which won't pull body-parser. The only genuine way to resolve this is to maintain your own types, but in the meantime, mediocre DefinitelyTyped versioning is unlikely to hurt anybody.

It should be noted that types packages do not pull non-types dependencies. It would be the responsibility of DT's Express maintainers to ensure any severe versioning problems are mitigated.

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

Successfully merging this pull request may close these issues.

None yet

2 participants