We should probably still throw an error upon assignment to ctx.session

I'm guessing the type of ctx.session shouldn't be changed to include undefiend, right?

I think so, too. It would be really annoying to work with. Then again ... we either have to accept that the types are wrong (ctx.session is absent but the types say it's not), or we need to retain the existing behaviour of throwing an error when the property is accessed incorrectly. The latter would be justifyable after #6.

I get that it could be kind of annoying, but with filtering it'd be easy to get rid of updates missing the session object. We could even export a predicate ourselves, especially after we move sessions to a dedicated plugin

What are you envisioning? bot.filter(hasSession)?

What are you envisioning? bot.filter(hasSession)?

yep, something like that

That feels a bit annoying, don't you think?

That feels a bit annoying, don't you think?

not really. the runtime check is already necessary for stuff that depend on the existence of a session key, well just reflect that in the types.

I'd just like to find a way to make it unnecessary to check things at runtime. The current implementation doesn't need that and it would feel like a step back to require this from now on

The current implementation doesn't need that

I might be missing something here, but I don't really get how this is true. isn't the whole purpose of this PQ to make the check easier? today, if you don't perform that runtime check, you risk getting an error

I don't think you're missing something. It's just really annoying to be forced to check for undefined. It's also really annoying that there is not way to check for the presence of the object. In essence, in order to have an ergonomic API, we need ctx.session to be both present and optional at the same time. That's a contradiction. How do we resolve this?

• Compromise on the side of using ctx.session in the common case, and let ctx.session be optional (feat: allow ctx.session to be undefined #2)
• Compromise on the side of checking the the presence of ctx.session in the rare case, and keep on throwing an error (=ignore this issue)
• Have ctx.session be present and still provide a way to check if it will throw, such as one of the following
  • ctx.sessionKey === undefined
  • ctx.hasSession === true
  • hasSession(ctx) === true
  • others

I honestly think the most ergonomic choice is 1.

It's true to the nature of the session property, the types don't lie, the runtime check is easy and, if you skip it, TS will yell at you, preventing unexpected exceptions.

It's the least footgunish of the three approaches IMO.

Most ergonomic?

ctx.session.count++

becomes

(ctx.session ??= { prop: 0 }).prop++

or less cryptic

ctx.session ??= { prop: 0 }
ctx.session.prop++

and in both cases, 100 % of the added code is just dead code. There is no code path that actually causes the session to be initialised here. It's purely an addition to satisfy TypeScript.

Also, note that the initialiser needs to list every single field in the session. Developers either need to make all fields optional and then add a second layer of checking, or they might resort to calling initial in every handler in order to reuse the setup of large session objects.

This needs to be done in every single place anything is done with the session. The migration efforts are huge.

There may be reasons to go with 1 but being ergonomic is not among them :D

I meant it's ergonomic because you can eliminate all that with a very simple .filter, then all the checking is gone, and the types are correct. This is even easier if we provide a hasSession predicate that narrows the context type.

Maybe it's not right to say it's the more ergonomic, but then it's the most correct, while still allowing easy checking.

I hate the idea of the types telling me ctx.session is there when it's possible it'll be undefined, I'd still have to check every time I use it, but I'll only know that if I read the docs (we know many people don't) or know the plug-in well, or fall into this pit at least once.

To be fair, with any of the approaches, we still have to check every time we use it, otherwise we might get "cannot read [...] of undefined" errors, or get an exception thrown at us.

I don't really see why having a property that's possibly undefined and easy to filter away is bad. It goes along every other optional property in Context, actually. You'll always have to narrow tb context type down, this is not very different from it.

The best I can think of in terms of DX and type safety is:

• Mark session as possibly undefined
• Add ctx.hasSession for when people want to check in place instead of filtering. This could be a property the plug-in sets, and we can tell TS ctx.session only exists when this is true.
• Export a hasSession predicate for easy .filter + context narrowing. It'd use ctx.hasSession under the hood and return ctx is C & { session: SessionData }.

Usage options would be:

1. With filter + predicate:

bot.filter(hasSession)
   .message((ctx, next) => {
      ctx.session.count++
      return next()
    })

2. With direct ctx.hasSession usage:

bot.message((ctx, next) => {
  if (!ctx.hasSession) {
    return ctx.sendMessage('Hi nameless')
  }

  ctx.session.count++

  return next()
})

We could maybe even drop 1 and stick with 2, letting people make their own predicates, depending on whether they want to handle rh existence or absence of a session. My point is: it's better DX to require the user to filter this out and then use the property safely then to let it the property return undefined or throw without the types telling the user that's possible.