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: allow getSessionKey to use extended ctx type #390

Merged
merged 8 commits into from
Mar 12, 2023

Conversation

KnightNiwrem
Copy link
Contributor

As of now, we are unable to use narrowed Context type for getSessionKey because the parameter type of ctx is fixed to Context - TS Playground

This change attempts to provide getSessionKey's ctx parameter with the narrowed Context type so that the following can be accomplished.

Screenshot 2023-03-11 at 12 31 47 PM

@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (6b67444) 34.95% compared to head (1f92a6e) 34.95%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #390   +/-   ##
=======================================
  Coverage   34.95%   34.95%           
=======================================
  Files          16       16           
  Lines        4806     4806           
  Branches      168      168           
=======================================
  Hits         1680     1680           
  Misses       3124     3124           
  Partials        2        2           
Impacted Files Coverage Δ
src/convenience/session.ts 96.88% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@KnorpelSenf
Copy link
Member

I remember trying this but due to some covariance/contravariance problems these changes would break a lot of bots. It is a conscious decision not to have it parameterised.

Perhaps I was wrong, so we could well spend some time on testing this once more. I'm just very careful here.

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I no longer know what the issue was. Let's ship this and see if this breaks anything. There is a chance that I am confusing this with initial, which could take a context object but does not.

@EdJoPaTo it would be helpful to have you test these changes on a few bots of yours.

One thing we certainly have to change is the availability of ctx.session inside getSessionKey:

src/convenience/session.ts Outdated Show resolved Hide resolved
src/convenience/session.ts Outdated Show resolved Hide resolved
@KnorpelSenf
Copy link
Member

Also, please fixup CI.

@KnorpelSenf KnorpelSenf changed the title Allow getSessionKey to use extended ctx type feat: allow getSessionKey to use extended ctx type Mar 11, 2023
@KnorpelSenf
Copy link
Member

Also, please fixup CI.

nvm, I did this now

Copy link
Member

@KnorpelSenf KnorpelSenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! 🎉

@KnorpelSenf KnorpelSenf merged commit 299ca4e into main Mar 12, 2023
@KnorpelSenf KnorpelSenf deleted the custom-getsessionkey-ctx-type branch March 12, 2023 09:23
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.

None yet

3 participants