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

New TypeScript typings #93

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HoldYourWaffle
Copy link
Contributor

The current DefinitelyTyped typings are horribly out of date and frankly unusable. To fix this I created new typings, but since DefinitelyTyped is a mess and it's generally preferable to include typings directly in the package I'm creating the PR here.

The typings are not finished yet, there's still a lot of any types (which basically means 'unknown' in case you're unfamiliar with TypeScript) and I'm not entirely confident I included all functions*.
I was hoping you could help me with the remaining any types and merge this, so that this library will be usable again in a TypeScript environment.



* On a side note, why are you directly modifying the prototype instead of using the class syntax? I'm relatively new to JS (at least up from ES5), and as far as I can tell there isn't really a reason for doing this. Am I missing somethings? I'd love to learn!

@chill117
Copy link
Owner

Hello! Thanks for taking the time to create a PR. I am not so familiar with TypeScript, but I am open to including these definitions in the project. Are there any best-practices from TypeScript-land for testing / validating the type definitions (e.g for completeness and/or correctness)?

  • On a side note, why are you directly modifying the prototype instead of using the class syntax? I'm relatively new to JS (at least up from ES5), and as far as I can tell there isn't really a reason for doing this. Am I missing somethings? I'd love to learn!

The class syntax is es6. This project is strictly es5 at the moment. I prefer not to mix the two.

@HoldYourWaffle
Copy link
Contributor Author

Are there any best-practices from TypeScript-land for testing / validating the type definitions (e.g for completeness and/or correctness)?

I don't know of any, but I'm sure projects like DefinitelyTyped have some testing method. I'll look into it.


I also have some additional questions for you:

  • What kind of funtions are clearExpiredSessions, setExpirationInterval and clearExpirationInterval? Should they be publicly available?
  • Wouldn't it be better to have mysql, mysql2 and express-session as peer dependencies?

@HoldYourWaffle
Copy link
Contributor Author

To make sure these definitions stay compatibly with express-session we should probably wait until express-session/#656 (new definitions) is either merged or rejected. If it's merged I'll make this definition compatible with the new, improved and included express-session definitions rather than the outdated (and wrong) definitions from DT.

@HoldYourWaffle
Copy link
Contributor Author

For anyone wanting to use these definitions already: I have published my fork as a separate package.

@chill117
Copy link
Owner

@HoldYourWaffle What's the status on this? Have the upstreams merged your other PRs?

@HoldYourWaffle
Copy link
Contributor Author

express-session has not merged my PR yet, but I have just posted a "reminder" comment.

@HoldYourWaffle
Copy link
Contributor Author

Because express-session will need to do a major-version-update to include the type definitions I have (temporarily) opted to update the definitions over at DT.

@HoldYourWaffle
Copy link
Contributor Author

Just to give a little status update, more than a year later this 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
Contributor Author

It's been 84 years

DefinitelyTyped/DefinitelyTyped#46576 has finally been merged!
There have been some major changes (read: improvements) so these definitions most likely are not 100% compatible. I'll try to update them somewhere this week.

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.

2 participants