-
Notifications
You must be signed in to change notification settings - Fork 28
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
[y sweet] support optional initialClientToken for YSweetProvider #319
[y sweet] support optional initialClientToken for YSweetProvider #319
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe pull request introduces significant updates to the In Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
js-pkg/client/src/provider.ts (2)
530-533
: LGTM: UpdatedySweetProviderWrapper
function signatureThe changes to the function signature and parameter handling are well-implemented:
- Renaming
providerParams
towrapperParams
more accurately represents the function's purpose.- Destructuring
initialClientToken
allows for its separate handling.Consider renaming the
providerParams
variable on line 533 to something likeremainingParams
orotherParams
to better reflect its content after destructuringinitialClientToken
.
544-544
: LGTM: ImplementedinitialClientToken
handlingThe implementation of
initialClientToken
handling is well done:
- It uses the nullish coalescing operator (
??
) to elegantly handle the case wheninitialClientToken
is not provided.- The logic maintains the existing functionality of fetching a new token when necessary.
This change successfully implements the PR objective of allowing the use of an initial client token when available.
For improved readability, consider extracting the token retrieval logic into a separate function:
function getInitialToken(initialToken: ClientToken | undefined, authEndpoint: AuthEndpoint, roomname: string): Promise<ClientToken> { return initialToken ?? getClientToken(authEndpoint, roomname); } // Usage let _clientToken = await getInitialToken(initialClientToken, authEndpoint, roomname);This extraction would make the main function body cleaner and the token retrieval logic more reusable.
js-pkg/react/src/main.tsx (1)
Line range hint
194-205
: IncludeauthEndpoint
andinitialClientToken
in the dependency array of theuseEffect
.The
useEffect
hook depends onauthEndpoint
andinitialClientToken
, but they are not included in the dependency array. Omitting these dependencies can lead to issues when eitherauthEndpoint
orinitialClientToken
changes, as the effect won't re-run to reflect the updated values.Apply this diff to include all necessary dependencies:
useEffect(() => { let canceled = false let provider: YSweetProviderWithClientToken | null = null const doc = new Y.Doc() ;(async () => { provider = await createYjsProvider(doc, docId, authEndpoint, { initialClientToken, // TODO: this disables local cross-tab communication, which makes // debugging easier, but should be re-enabled eventually disableBc: true, }) if (canceled) { provider.destroy() return } setCtx({ doc, provider }) })() return () => { canceled = true provider?.destroy() doc.destroy() } - }, [docId]) + }, [docId, authEndpoint, initialClientToken])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (2)
- js-pkg/client/src/provider.ts (3 hunks)
- js-pkg/react/src/main.tsx (3 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: rolyatmax PR: jamsocket/y-sweet#294 File: js-pkg/react/src/main.tsx:0-0 Timestamp: 2024-09-30T18:35:52.941Z Learning: The `getProvider` logic is the main difference between `YDocProvider` and `YDocProviderFromClientToken` components, so refactoring to eliminate duplication may not be appropriate.
Learnt from: rolyatmax PR: jamsocket/y-sweet#294 File: js-pkg/react/src/main.tsx:0-0 Timestamp: 2024-10-08T15:32:21.105Z Learning: The `getProvider` logic is the main difference between `YDocProvider` and `YDocProviderFromClientToken` components, so refactoring to eliminate duplication may not be appropriate.
js-pkg/react/src/main.tsx (2)
Learnt from: rolyatmax PR: jamsocket/y-sweet#294 File: js-pkg/react/src/main.tsx:0-0 Timestamp: 2024-10-08T15:32:21.105Z Learning: The `getProvider` logic is the main difference between `YDocProvider` and `YDocProviderFromClientToken` components, so refactoring to eliminate duplication may not be appropriate.
Learnt from: rolyatmax PR: jamsocket/y-sweet#294 File: js-pkg/react/src/main.tsx:0-0 Timestamp: 2024-09-30T18:35:52.941Z Learning: The `getProvider` logic is the main difference between `YDocProvider` and `YDocProviderFromClientToken` components, so refactoring to eliminate duplication may not be appropriate.
🔇 Additional comments (3)
js-pkg/client/src/provider.ts (2)
227-227
: LGTM: Addition ofinitialClientToken
toYSweetProviderParams
The addition of the optional
initialClientToken
property toYSweetProviderParams
aligns well with the PR objective of allowing developers to provide a client token at render time. This change enhances the flexibility of theYSweetProvider
while maintaining backwards compatibility.
Line range hint
227-544
: Summary: Successfully implemented optionalinitialClientToken
supportThe changes in this file successfully implement the PR objective of supporting an optional initial client token for the
YSweetProvider
. The implementation is well-thought-out, maintaining backwards compatibility and integrating smoothly with the existing codebase.Key points:
- Added
initialClientToken
toYSweetProviderParams
.- Updated
ySweetProviderWrapper
to handle the newinitialClientToken
.- Implemented logic to use
initialClientToken
when available or fetch a new token otherwise.The code is clean, and the changes are minimal yet effective. The minor suggestions provided in the review comments could further enhance readability and maintainability.
js-pkg/react/src/main.tsx (1)
175-183
: Documentation clarity forYDocProviderProps
.The updated documentation for
docId
,authEndpoint
, andinitialClientToken
enhances clarity. The descriptions accurately reflect the purpose and usage of each property.
/** An optional initial client token to use for the Y.Doc. This will be used initially, and if | ||
* the client token expires, the `authEndpoint` will be called to get a new client token. */ | ||
initialClientToken?: ClientToken |
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.
🛠️ Refactor suggestion
Evaluate merging YDocProviderFromClientToken
into YDocProvider
.
With the addition of the initialClientToken
property to YDocProvider
, it appears that the functionality overlaps significantly with YDocProviderFromClientToken
. This presents an opportunity to reduce code duplication by consolidating these components.
Based on previous learnings, refactoring was deemed inappropriate due to differences in the getProvider
logic. However, now that YDocProvider
can accept an initialClientToken
, please reassess whether combining the components is feasible and beneficial.
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.
YDocProviderFromClientToken
doesn't exist
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.
@rolyatmax, thank you for pointing out that YDocProviderFromClientToken
doesn't exist. My apologies for the oversight. It seems that the previous learnings about YDocProviderFromClientToken
may no longer be relevant. Would you like me to remove these learnings from the knowledge base?
(^_^;)
🧠 Learnings used
Learnt from: rolyatmax
PR: jamsocket/y-sweet#294
File: js-pkg/react/src/main.tsx:0-0
Timestamp: 2024-09-30T18:35:52.941Z
Learning: The `getProvider` logic is the main difference between `YDocProvider` and `YDocProviderFromClientToken` components, so refactoring to eliminate duplication may not be appropriate.
Learnt from: rolyatmax
PR: jamsocket/y-sweet#294
File: js-pkg/react/src/main.tsx:0-0
Timestamp: 2024-10-08T15:32:21.105Z
Learning: The `getProvider` logic is the main difference between `YDocProvider` and `YDocProviderFromClientToken` components, so refactoring to eliminate duplication may not be appropriate.
This updates the
YSweetProvider
(and the React<YDocProvider>
) to take an optionalinitialClientToken
. This can be used to cut out one server roundtrip if the client token is obtained at render time.