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

feat: let ctx.session be undefined for missing session keys #5

Open
Tracked by #675
KnorpelSenf opened this issue Dec 15, 2024 · 11 comments · May be fixed by #2
Open
Tracked by #675

feat: let ctx.session be undefined for missing session keys #5

KnorpelSenf opened this issue Dec 15, 2024 · 11 comments · May be fixed by #2

Comments

@KnorpelSenf
Copy link
Member

KnorpelSenf commented Dec 15, 2024

The fact that an error is thrown upon property access is really annoying. There is no good way of knowing whether session data exists, so we should change this behaviour.

@KnorpelSenf KnorpelSenf changed the title feat: let ctx.session be undefined for missing sessions feat: let ctx.session be undefined for missing session keys Dec 18, 2024
@KnorpelSenf
Copy link
Member Author

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

@roziscoding
Copy link
Contributor

roziscoding commented Mar 3, 2025

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

@KnorpelSenf
Copy link
Member Author

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.

@roziscoding
Copy link
Contributor

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

@KnorpelSenf
Copy link
Member Author

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

@roziscoding
Copy link
Contributor

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

yep, something like that

@KnorpelSenf
Copy link
Member Author

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

@roziscoding
Copy link
Contributor

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.

@KnorpelSenf
Copy link
Member Author

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

@roziscoding
Copy link
Contributor

roziscoding commented Mar 13, 2025

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

@KnorpelSenf
Copy link
Member Author

KnorpelSenf commented Mar 17, 2025

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

@KnorpelSenf KnorpelSenf linked a pull request Mar 17, 2025 that will close this issue
@KnorpelSenf KnorpelSenf transferred this issue from grammyjs/grammY Mar 17, 2025
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 a pull request may close this issue.

2 participants