TypeScript typings

[bangbang93]

No description provided.

[mormahr]

Hi,

thanks for contributing! Please undo the second commit (lts/carbon test) as it is not related to TypeScript support. I'll look closer into these changes in the coming days.

Thanks again! ☺️

[bangbang93]

OK, I removed the second commit.

[jcristovao]

This is a very welcome addition, and it seems to work fine, at least on the project I tested, thanks!
@mormahr , are the changes proposed by @bangbang93 enough for a merge? ;)

EDIT: actually, I stand corrected, I do have some issues regarding name conflicts (using Typescript 2.3.2)

[jcristovao]

You are importing Router to the scope, and then redefining a Router function, which at least on my version of typescript leads to a conflict

[bangbang93]

I'm using 2.5.3,and works fine.

[jcristovao]

I tried to upgrade to 2.6.2, and the problem still maintains itself:
node_modules/express-promise-router/index.d.ts(2,13): error TS2440: Import declaration conflicts with local declaration of 'Router'..

Still, the fix is pretty simple:

import {Router as RouterExpress, RouterOptions} from 'express'

     function Router(options?: RouterOptions): RouterExpress
 
     export = Router
 }

What do you think?

[mormahr]

Unfortunately i haven't used TypeScript (active FlowType user), but i still would want to try this out before merging. I'll see when i can fit it in :)

[mormahr]

Hi,

i'm exploring options to test the TypeScript definition as part of the test suite. So far i've found typescript-definition-tester and wanted to know what you think about it and what kind of example definition one would want to write. I'm unfamiliar in TypeScript land. Essentially i'd like to at least have an automated test to verify that the .d.ts file is valid, it would be much better if we could actually test it's typing though.

I created a really simple test case using typescript-definition-tester (just importing the definition basically) and stumbled upon the same error that @jcristovao got.

@bangbang93 do you think it's possible to implement his workaround?

AssertionError: Semantic: /Users/moritzmahringer/Documents/Projekte/express-promise-router/index.d.ts (2,13): Import declaration conflicts with local declaration of 'Router'.

When i have some spare time i might bring my initial test into a presentable form and add it here.

Moritz

[bangbang93]

I prefer to rename function as "PromiseRouter" rather than "import as"

This should works fine now.

[mormahr]

Hi,

we need tests for this before merging it. I prepared tests but can't commit/push it to your PR. Did you disable "Allow edits from maintainers"?

Moritz

[bangbang93]

well……It's checked

[mormahr]

Mhm i cloned your repo and committed there, don't know if thats the intended way to do this, i thought i could just push on pr/43.. Oh well 🤷🏻‍♂️

I'll look into why the test fails this weekend!

Moritz

[joepie91]

Was this supposed to add @types/express@^4.0.0 as a peerDependency? This seems a little odd, considering many users won't be using TypeScript (but it still causes warnings for them) and there doesn't seem to be a concrete purpose to this addition.

EDIT: As an aside, what's the breaking change that prompted the major version bump?

[mormahr]

Oh you're totally right! I don't actually know what the best practice is here, but i suppose we can just remove the peerDependency.

Are there any downsides for TypeScript users? Otherwise i'd push a fix in a few minutes.

[mormahr]

@joepie91

As an aside, what's the breaking change that prompted the major version bump?

To be honest, there is not enough testing from users / contributors of this library. So I released it as a major version to reduce the risk of breaking anything as many people still don't use pinned versions. I do plan to change this for the next releases. But i would like to have a few people join me in testing changes. I deploy these changes (-beta, -rc, etc.) to production on my projects before releasing the final version, but my testing might not be enough. Basically i'm over cautious because i'm the only tester.

[joepie91]

Fair enough.

For what it's worth, the general approach with semantic versioning is to treat the documentation as authorative; that is, whatever the documentation says the API is should be what the API actually is, and if the documented API hasn't broken in an incompatible way then it's not a breaking release. Bugfixes ("bring the actual behaviour in line with the documented behaviour") are then considered a patch release.

This solves a number of conundrums:

1. What if you made an implementation error and you want to push a fix to all your users, since they will probably have designed against the documentation? Making it a major bump would mean users don't get the updates, so the sensible thing is to make it a patch bump.
2. What if you changed some sort of undocumented API, especially if the API was accidentally exposed? Making this a major bump would break the automatic upgrade process for everybody else (despite there being almost no chance of breakage), and the user really shouldn't have depended on an undocumented API in the first place, so that's also a patch bump.
3. And then your situation: what if you're not sure whether there are any bugs? If you simply go by the documented API, then any new reported issues can just be fixed in a patch release with limited impact. Doing otherwise would make versioning and upgrade management untenable as a maintainer.

I wouldn't worry too much about breakage due to not locking package versions; any automatic updates happen during deployment steps anyway, when a human is paying attention and can test their deployment. Those who are hesitant about automatically getting updates that might break their application (and who can afford the time to vet every update manually!) can already use version pinning anyway.

I actually often recommend that people don't use version pinning, because the cost of continuously tracking and vetting updates is considerably higher than dealing with the occasional breakage, and most people don't have teams of developers to do this task around the clock.

But in the end it's up to the developer which side of the tradeoff to pick, anyway - and in that context it's totally okay to release a patch/minor release even if you think there may still be undiscovered bugs. It's desirable, even; those who opt into automatic updates won't automatically get the updates if they're stuck on the wrong major version.

Either way, thanks for the speedy fix :)