Import & update typings from DefinitelyTyped

[HoldYourWaffle]

The current DefinitelyTyped typescript definitions for this module are a mess. They're needlessly convoluted and in some ways outdated or wrong. Because my definitions would be a breaking change and because DT is a mess in general (giant mono-repo, having to install another package) I suggest merging these definitions into the main repository.

I made 2 commits to make the difference between the DT definitions and mine clearer, I'd be happy to squash them before this is merged.

Changes relative to the DT definition

• regenerate, load and createSession are not functions on Store. I'm not sure why they were there in the first place, I think Session has similar methods but they aren't a proper match.
• Reuse Express.CookieOptions for SessionCookie¹
• SessionData no longer has a string => any index type. This makes it possible to use declaration merging to allow the user to type their own sessions. This is also seen on express's Request type.
• There is no BaseMemoryStore, MemoryStore just extends Store
• A less convoluted type hierarchy

Notes

• I suggest removing the optional modifier on Express.Request.session and Express.Request.sessionID. Although they're technically 'optional' in the sense that they don't exist before you use your session middleware it's really inconvenient when working with strict null checking enabled. If you have used the middleware these properties are guaranteed to be there, making any checks redundant. Because of this I'd suggest going for convenience rather than accuracy in this instance.
• ¹I'm definitely not sure about the types of the cookie related properties. There seems to be a container Cookie class here but I'm not sure how it's used and how to represent it in these definitions. I made it an interface which extends Express.CookieOptions for now but I'm pretty sure this neither accurate nor semantically correct.

@horiuchi, @jacobbogers, @builtinnya and @ry7n, you are credited as the original authors of the DT definitions. I have a couple of questions about why they are the way they are:

• Why is the type hierarchy so convoluted? I couldn't find a reference to anything resembling a BaseMemoryStore and several inheritances redefined common functions.
• What do you think the cookie types should be?
• Are there actually good reasons for why the convoluted hierarchies are that way, in other words: did I miss something?

[HoldYourWaffle]

I just noticed that although the readme says that a store must be an EventEmitter there's no mention of why they have to be and how it's actually used. I've never used EventEmitters before, but from what I can tell by a quick google search there doesn't seem to be a reason for this requirement. Even the connect-redis implementation that's set as an example doesn't seem to implement it (although I could just be missing something).

Is the readme wrong or is being an EventEmitter an actual requirement? If so, why do so many stores not implement it (including the one used as example in the readme)?

[HoldYourWaffle]

Turns out declaration merging doesn't work properly with these definitions, currently investigating.

[HoldYourWaffle]

I have addressed your concerns, including a crude concept of additional documentation in the readme.

What do you think about my suggestion to remove the optional modifier on Request.session and Request.sessionID? Seems like a reasonable trade-off to me, especially with a note on it in the readme (included in the current concept). And what about the EventEmitter requirement? I haven't found a reason why it's needed nor a store that actually fulfills it.

[dougwilson]

So i have not finished reviewing yet, just things I found from a first look.

What do you think about my suggestion to remove the optional modifier on Request.session and Request.sessionID?

I don't know what this means, can you elaborate if you need an answer from me?

And what about the EventEmitter requirement? I haven't found a reason why it's needed nor a store that actually fulfills it.

It is required. Try making a store without it and it will fail immediately since this module attempts to add event listeners to the store during set up. Not sure what the confusion is.

[HoldYourWaffle]

I don't know what this means, can you elaborate if you need an answer from me?

Typescript employs strict null/undefined checking, which basically means that the compiler screams at you for using a nullable/undefinedable reference without the proper checks. It's really inconvenient in this case, although technically correct to have the session and sessionID properties be 'undefinedable' (optional). The original DT definitions did list these properties as optional, but I'm pretty sure other middleware type definitions don't do this. Either way it's just really inconvenient and messy to have these properties as 'optional', though technically correct. I hope that's enough elaboration, if not I'm sure one of the original authors will be happy to comment on this matter.

It is required. Try making a store without it and it will fail immediately since this module attempts to add event listeners to the store during set up. Not sure what the confusion is.

Apologies, I completely missed this line which effectively causes all store implementations to implicitly extend EventEmitter. I have updated the definitions accordingly.

[dougwilson]

I hope that's enough elaboration, if not I'm sure one of the original authors will be happy to comment on this matter.

Well, I've never used typescript and still this seems over my head to be able to answer.

[HoldYourWaffle]

I found another possible mistake in the typings which boils down to the following: do the functions on the Store class expect a full-blown Session (including id, cookie, regenerate, etc.) or just the user-defined data properties? The DT definitions went for the latter (although I could just be misinterpreting the weird convoluted hierarchy), but I'm not sure that's correct as it seems pretty backwards.

[HoldYourWaffle]

I have decided to drop the optional modifier on session/sessionID, mainly because in 99% of the cases it's completely useless. It's just unnecessarily inconvenient, and it's safe to assume someone will actually register the middleware if they're using this module.

I also decided to give the length parameter of the callback passed to the function with the same name a type of just number. I can't think of a use-case where length would return anything else than a number without an error having occured (which in my opinion should result in an error being thrown). Although technically valid, I don't think it's worth it to bother typescript users with a nullable/undefinedable typing.

Lastly I took another shot at the whole cookie system hierarchy. I think I got it right this time, but you should definitely check my summary in the comment above in case I missed something.

Are there any more changes you'd like to see before this can get merged?

[dougwilson]

Thanks so much! I will take another review though them due to the number of changes to double check as well :+1:

As for next steps, ignoring the outcome of the review, it would be nice to...

1. Have a passing CI to merge
2. Have tests in some form to, at minimum, verify that the tsd is syntacticly correct since it is hand maintained
3. Is this safe to just land in 1.x? Not sure how landing this affects existing ts users.

[HoldYourWaffle]

Have a passing CI to merge

Don't ask me how but I never noticed that the tests are failing. No idea how that can happen by the addition of typings but I'll take a look.

Have tests in some form to, at minimum, verify that the tsd is syntacticly correct since it is hand maintained

I'll see how DefinitelyTyped does this, since they're the biggest (hand-maintained) repository of TypeScript definitions.

Is this safe to just land in 1.x? Not sure how landing this affects existing ts users.

No idea actually. It's probably going to conflict somehow with the DefinitelyTyped version but I'm not sure how well the TypeScript compiler handles duplicate typings. It would make sense for it to only use the in-package definitions but I don't think that's actually how it works. However, if this gets merged the DefinitelyTyped package will ultimately be deprecated/removed providing existing TS users with a nice little warning that'll tell them there are new (and improved) definitions. As for safety, I think these definitions are under normal usage circumstances compatible (since they both describe the same module and the DT one technically did 'work' although badly).

[dougwilson]

So the safe to land in 1.x is, overall, my main concern. Is there any way to gather what would happen to an existing app?

[HoldYourWaffle]

So the safe to land in 1.x is, overall, my main concern. Is there any way to gather what would happen to an existing app?

Do you know a codebase I could test on?

[dougwilson]

No, I have not used TypeScript before.

[HoldYourWaffle]

Turns out CI is only failing in Node v0.8, probably because it's just too old to install the @types/express and @types/node packages (although I'm not sure why those packages would cause issues). If you don't mind I can just remove Node v0.8 from the test matrix, which I don't think will be that big of an issue since it's almost 7 years old now. This comment suggests that only 0.04% of people were using Node v0.8 3 years ago (not sure where this number comes from but I really don't think the actual percentage will be much higher).

[dougwilson]

So our (and my) policy is that it would require a new major version to drop support for a Node.js version. We can drop it, but it would then push landing this to 2.0, placing it behind the queue of 1.x PRs to land. I'm just providing this for transparency.

[HoldYourWaffle]

I did some more digging, seems like the npm version used by travis with node v0.8 is not supported anymore. Not sure why it still works for other commits, maybe travis caches installed packages?

I tested replacing DT typings with these on about 10 repositories listed on npm that depend on this package, got pretty inconclusive results. Most repo's were broken, but from what did work I can already tell that stuff certainly can go wrong. This seems to be mostly import-type related (which makes sense).

So since it seems like the major version should be bumped anyway for this, would it be alright to remove v0.8 from travis? Also, how long will it be until v2.0 is released? There's no rush, I can use my own fork in the meantime, but it's nice to have an indication. It would also help with the process of deprecating/removing the old DT definitions.

[dougwilson]

So since it seems like the major version should be bumped anyway for this, would it be alright to remove v0.8 from travis?

Yes, you are welcome to do so.

Also, how long will it be until v2.0 is released?

I need to go through the PRs created before yours to land still. Most (if not all) can land on 1.x. I will rebase this PR into the 2.0 branch when it is cut. I don't have a specific timeline for this.

[HoldYourWaffle]

That makes sense, I'll use my fork for now.

Meanwhile I'll try to convince the folks over at DT to deprecate the old definitions.

[dougwilson]

And to add that since this now cannot land until 2.0, the question about landing in 1.x for existing TypeScript users is obvious irrelevant :)

[HoldYourWaffle]

I have squashed all commits down to only 2: the actual definitions & the support drop for v0.8

[HoldYourWaffle]

In case anyone wants to use these definitions before the next major release without the hassle of git dependencies in node: I have published my fork here.

[HoldYourWaffle]

It's been 8 months now, is there any chance of this getting merged soon? Fixed typings for downstream session stores depend on this PR, so it'd be nice to have a solution that doesn't involve forked dependencies.

Little note to myself: notify https://github.com/JLuboff/connect-mssql-v2/issues/10 when this is merged.

[dougwilson]

https://github.com/expressjs/session/pull/656#issuecomment-489245845

[voxpelli]

Wouldn’t it be possible to make the types optional dependencies?

They are probably failing because they are scoped dependencies that such an old npm version cannot handle?

Landing this in 1.x would be preferable as any future 2.x is just that – something that eventually happens at some point in the future.

[dougwilson]

I'm not super familiar with typings, so can't really say. Besides the node.js change in this PR, I would assume if these landed in a 1.x version it would still break typescript projects, since the typings here are not compatible with existing typings folks are using, is that a correct assumption?

[HoldYourWaffle]

Wouldn’t it be possible to make the types optional dependencies? They are probably failing because they are scoped dependencies that such an old npm version cannot handle?

I don't fully understand what you're trying to say here.

since the typings here are not compatible with existing typings folks are using, is that a correct assumption?

I don't fully remember, but I'd be surprised if these new typings don't break something.

These changes definitely need to go into a new major version, but I agree with what @voxpelli says (I think). It's been 8 months since I submitted this PR, and (as far as I can tell) v2 hasn't come any closer. You say you want to wait with releasing v2 until all v1-PR's have 'landed', which makes a lot of sense, but if there's no work being done to reach this point it could take years to resolve this mess.

I'm not saying there's nothing happening, because I honestly have no idea if that's the case. I haven't used this module for several months, the only reason I checked in was because a downstream maintainer asked for a status update (https://github.com/chill117/express-mysql-session/pull/93#issuecomment-572961325).

The current typings mess is (most likely) affecting everyone who uses this library with TS, so I think it's fair to say that a quick solution would be appreciated by a lot of people. I don't want to rush you or anything, I'm just trying to help resolve this typings mess, in the hope that others won't have to go through the same troubles as me.

[HoldYourWaffle]

English is not my first language, so I'm sorry if I seem a bit blunt.

Perhaps it'd be possible to release v2 before all PR's have landed? This means that the new features won't be in v1 of course, but since the breaking changes are minimal updating shouldn't be a problem almost all, if not all, users.

[dougwilson]

If you really need a quick solution, why can the typings not be updated in definatelytyped? Wouldn't that solve everything at concern here? Since that is a separate dependency folks can pull in the version of types that work for them without breaking others and then the others have a chance to migrate? Especially since the types are for the 1.x of this module.

[dougwilson]

I'm ultimately not interested in releasing a major version of this module just to fix typings I never made. I'm happy to land them in here in conjunction with the other badly-needed changes. But I'm not sure why now the pressure is on a module and person who did not make a bunch of broken typings in the first place... this will be landed along with 2.0, but if you really need to "fix the mess" that is not even part of this module, land the fix in definitelytyped in the interim. Folks can use their package.json to refer to the relevant type definitions.

[dougwilson]

Note I have locked this thread temporarily while I can get some samesite cookie releases out tonight for folks and can resume this discussion at a later time. One thing to consider for later is who will keep the types up to date with changes in this module? Landing this before the other PRs will make them unlandable as-is since the types will be off. I just want to get those out of the way before we land the types so they are at least not just immediately out of data again. This week I will start a 2.0 beta branch to start getting things landed. We will use that as a test to see if there is anyone who helps keep the types up to date though those changes in the beta as a test for them going in the final release. That is just a proposal, and we can further discuss. I will open a meta issue so things are not lost.