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

Import + update typings from DefinitelyTyped #155

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HoldYourWaffle
Copy link

@HoldYourWaffle HoldYourWaffle commented Sep 29, 2019

express-session is getting new typings that are incompatible with the current ones on DefinitelyTyped.
I updated the typings for this module to stay compatible with these new upstream typings.

Because DefinitelyTyped is a mess and it's generally preferable to include typings directly in the package, I'm creating the PR here. If you don't want to include type definitions directly in the package I'll be happy to move this PR over to DT.

These updates should be backwards-compatible with those on DefinitelyTyped because I only added some missing definitions for functions.

@HoldYourWaffle
Copy link
Author

Shoot I misclicked and created the PR without finishing my comment. In case you're reading this in an email: be sure to read my edit.

@HoldYourWaffle
Copy link
Author

In case anyone wants to use these definitions before this is merged without the hassle of git dependencies in node: I have published my fork here (be sure to use my fork of express-session as well).

@voxpelli
Copy link
Owner

voxpelli commented Oct 1, 2019

I'm not opposed to adding type definitions 👍

Does these ones have a dependency upon expressjs/session#656 ?

@HoldYourWaffle
Copy link
Author

That's great to hear!

They shouldn't since I just added some methods, but I haven't explicitly tested it either (nor am I able to right now).

@voxpelli
Copy link
Owner

@HoldYourWaffle Did you have any success in the DefinitelyTyped and/or Express project?

@HoldYourWaffle
Copy link
Author

Not really unfortunately.
This was a couple of months ago, but if I remember correctly, there were some issues over at express concerning maintenance and a new major version. I tried putting the new definitions in DefinitelyTyped, but the bot kindof screwed me over.

@voxpelli voxpelli added this to the 7.0.0 milestone Jul 30, 2020
@HoldYourWaffle
Copy link
Author

Just to give a little status update, more than a year later the upstream issues still hasn't been resolved. I have just created a second-attempt PR over at DT, hoping to finally get this fixed as soon as possible.

@HoldYourWaffle
Copy link
Author

It has been more than a year, but I'm happy to announce that DefinitelyTyped/DefinitelyTyped#46576 was finally merged!

I'll try to update these definitions as soon as possible.

@voxpelli
Copy link
Owner

Cool @HoldYourWaffle, thanks for the effort!

Base automatically changed from master to main January 25, 2021 09:41
@obedm503
Copy link

any updates on this?

@voxpelli
Copy link
Owner

@obedm503 No, not from me, been holding back a bit to see how DefinitelyTyped/DefinitelyTyped#46576 has been working out, but should be something I should look into when I have some time

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

Successfully merging this pull request may close these issues.

3 participants