search

JLuboff / connect-mssql-v2

MS SQL Server session store for Express Session
MIT License
5 stars 7 forks source link

Type Definitions #10

Closed JLuboff closed 4 years ago

JLuboff commented 4 years ago

Need to straighten out type definitions. Currently, Store accepts an argument of session with type any and returns with type any. In turn, does not properly supply types.

bradtaniguchi commented 4 years ago

I was looking into this and found this repo, which implements another express-session store, but in typescript for typeORM it extends the Store class/type/instance. https://github.com/nykula/connect-typeorm/blob/master/src/app/TypeormStore/TypeormStore.ts#L59

I was messing around with the types and noticed all the function types need to be arrow functions rather than instance methods (what we have right now). I'm not 100% sure how these are mapped right now, but apparently everything is working as expected.

I'm going to update the types and change around how we declare the methods to be arrow functions and open a PR.

edit I did more digging into the types and noticed something. The arrow functions are defined via some type definitions, but they actually seem wrong, in that every implementation of the session store I've seen uses prototype IE instance methods (which sounds like the right way). I'm going to dig into it more and see if I can override it or something and then maybe we open an issue on the offending types :)

edit after even more digging I ran into this issue, which talks about having express-session handle its own types. This is suppose to land in version 2 of the lib, as it could break with the DT versions (which we are using).

Generally the DT versions are a mess, if that wasn't clear with all my above comments haha. Basically fixing the types now against the express-session types doesn't seem like the best idea due to all the underlying issues with the types in the first place.

As such the internal existing types should be good enough, I'll update just the public facing api to match with the express-session types.

JLuboff commented 4 years ago

@bradtaniguchi Thanks for the work on this! I was struggling trying to figure out the whole instance method vs instance function error. Your digging seems to have discovered why 👍 I'll go ahead and merge the PR and we'll keep a look out for updated express-session types, at which point we can update as needed.

HoldYourWaffle commented 4 years ago

after even more digging I ran into this issue, which talks about having express-session handle its own types. This is suppose to land in version 2 of the lib, as it could break with the DT versions (which we are using).

I'm the author of the PR you linked, and yes, the DT typings are an absolute mess. The PR to fix the typings for express-session has been on-hold until v2 for 8 months now, I'll post another comment as soon as it's merged.

HoldYourWaffle commented 4 years ago

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 commented 3 years ago

I'll post another comment as soon as it's merged.

It has been 20 months, but I'm happy to say that https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46576 has just been merged!

bradtaniguchi commented 3 years ago

@HoldYourWaffle Thanks! Seemed like a real battle to get it through!

Going to be looking into refactoring the types here with the new changes :D

Thanks for all the effort!