-
-
Notifications
You must be signed in to change notification settings - Fork 870
feat(context-storage): Add optional tryGetContext helper to context-storage middleware #4539
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4539 +/- ##
==========================================
- Coverage 91.52% 91.49% -0.03%
==========================================
Files 172 172
Lines 11221 11233 +12
Branches 3255 3258 +3
==========================================
+ Hits 10270 10278 +8
- Misses 950 954 +4
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR will resolve #4536. |
|
@AyushCoder9 Thank you for the PR! Hi @elibarzilay! What do you think of this PR? I think good. |
LGTM either way -- I specifically avoided thinking too much about a good name, to leave it up to people who might know the codebase better :) |
@elibarzilay are you merging this pr ? Tell me if I need to change something !! |
|
I'm just the person who made the feature request, I'm not a regular contributor... (Also, I'm guessing that it would be good to get a blessing from @marceloverdijk since he wrote the code.) |
@marceloverdijk check if this pr is good enough |
|
👍 I think it looks good (and a nice addition) |
glad to here that @marceloverdijk merge it if you liked it, would be an honour !! |
|
Hi @AyushCoder9 Can you handle the comment #4539 (comment)? If you don't react to it, we can't merge this. |
@yusukebe I've updated the PR to rename |
yusukebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Thank you!! We are planning to include this change in the next minor version. So, I'll merge this later! |
introduce getContextIfAny, which returns the stored context or undefined without throwing
make the existing getContext delegate to the softer helper so behavior stays unchanged when no context exists
expand the context-storage tests to cover both success and missing-context scenarios for the new helper