-
Notifications
You must be signed in to change notification settings - Fork 11
feat(KNO-9338): add branch configuration options #696
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(KNO-9338): add branch configuration options #696
Conversation
🦋 Changeset detectedLatest commit: 3173ae7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
kylemcd
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.
This is so good! Appreciate all the docs and the tests 🙏
| { id: user.id }, | ||
| undefined, // userToken when needed | ||
| { branch: "my-branch-slug" }, |
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.
Dang, really wish we would have just put these 3 into a single object when creating this hook originally :(
| ) { | ||
| this.host = options.host || DEFAULT_HOST; | ||
| this.logLevel = options.logLevel; | ||
| this.branch = options.branch || undefined; |
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.
Bug: Null Branch Value Causes Header and Parameter Errors
The ApiClient class initializes this.branch to null when the branch option is undefined. This null value is then incorrectly used for the X-Knock-Branch HTTP header and the branch_slug WebSocket parameter, which should be strings or omitted.
Additional Locations (2)
6adce9f to
3173ae7
Compare
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.
Bug: Knock instance reuses outdated configuration.
When userId or userToken changes simultaneously with branch (or host/logLevel), the hook reuses the existing Knock instance instead of creating a new one. Since branch is readonly and set during construction, the old branch value persists. The condition should verify that configuration options haven't changed before taking the early return path.
packages/react-core/src/modules/core/hooks/useAuthenticatedKnockClient.ts#L67-L76
javascript/packages/react-core/src/modules/core/hooks/useAuthenticatedKnockClient.ts
Lines 67 to 76 in 3173ae7
| if ( | |
| currentKnock && | |
| currentKnock.isAuthenticated() && | |
| (currentKnock.userId !== userId || currentKnock.userToken !== userToken) | |
| ) { | |
| authenticateWithOptions( | |
| currentKnock, | |
| stableUserIdOrObject, | |
| userToken, | |
| stableOptions, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #696 +/- ##
==========================================
- Coverage 65.53% 65.49% -0.05%
==========================================
Files 189 189
Lines 7771 7793 +22
Branches 954 956 +2
==========================================
+ Hits 5093 5104 +11
- Misses 2651 2662 +11
Partials 27 27
|

Description
This PR updates our client SDK and React SDK to support specifying a
branchoption in theKnockclient, the<KnockProvider>component, and theuseAuthenticatedKnockClienthook.I’m marking this PR with the “do not merge” label until we’re ready to ship this.
Checklist