Rewrite provider #352
Merged
Rewrite provider #352
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to e28fca4 in 59 seconds
More details
- Looked at
1199lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. js-pkg/client/src/main.ts:13
- Draft comment:
Update the JSDoc comment to reflect thatcreateYjsProvidernow returns aYSweetProviderinstead of aPromise<YSweetProviderWithClientToken>. - Reason this comment was not posted:
Comment was on unchanged code.
2. tests/src/index.test.ts:25
- Draft comment:
Consider re-adding thedisableBcoption to ensure tests run in isolated environments, as the removal might affect tests that rely on isolated environments for each provider instance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative and does not provide strong evidence that the removal ofdisableBcwill cause issues. It suggests a change without clear justification. According to the rules, speculative comments should be removed unless there is strong evidence of an issue.
I might be overlooking the potential impact of removingdisableBcon the test environment, but without strong evidence, the comment remains speculative.
The rules prioritize strong evidence over speculation, so without clear evidence, the comment should be removed.
The comment should be deleted because it is speculative and lacks strong evidence of an issue.
3. js-pkg/client/src/provider.ts:236
- Draft comment:
Ensure that closing the existing websocket before setting up a new one inbindWebsocketis the intended behavior and doesn't cause issues elsewhere in the application. - Reason this comment was not posted:
Confidence changes required:50%
Injs-pkg/client/src/provider.ts, thebindWebsocketmethod is closing the existing websocket before setting up a new one. This is a good practice to ensure no lingering connections, but it should be confirmed that this behavior is intended and doesn't cause issues elsewhere.
4. js-pkg/react/src/main.tsx:67
- Draft comment:
Ensure thatclientTokenis always set when expected inuseYSweetDebugUrlto prevent unexpected empty URL returns. - Reason this comment was not posted:
Confidence changes required:50%
Injs-pkg/react/src/main.tsx, theuseYSweetDebugUrlfunction checks forprovider.clientTokenbefore returning a URL. This is a good check to prevent errors, but it should be confirmed thatclientTokenis always set when expected.
Workflow ID: wflow_VK7ba20yFHDmQELd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on f057dde in 8 seconds
More details
- Looked at
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. debugger/src/app/debugger.tsx:28
- Draft comment:
Ensure that removing the WebSocket URL display does not affect debugging capabilities. Consider providing an alternative way to access this information if needed. - Reason this comment was not posted:
Confidence changes required:50%
The removal of the WebSocket URL display might affect debugging capabilities. It's important to ensure that this information is not needed elsewhere or that there is an alternative way to access it if necessary.
Workflow ID: wflow_HbyJgZJHcYaYrkrc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d5c9923 in 8 seconds
More details
- Looked at
40lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:65
- Draft comment:
TheresyncIntervalparameter was removed fromYSweetProviderParams. Ensure that this removal does not affect any functionality that relied on this parameter. - Reason this comment was not posted:
Confidence changes required:50%
TheresyncIntervalparameter was removed from theYSweetProviderParamstype, but there is no indication of how this affects the functionality. If this parameter was used elsewhere, it might cause issues.
Workflow ID: wflow_7uJoGTnHGd9KWwqR
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
This was referenced Dec 11, 2024
jakelazaroff
requested changes
Dec 11, 2024
js-pkg/client/src/provider.ts
Outdated
Comment on lines
422
to
423
| if (once) { | ||
| this.once(type, listener) |
There was a problem hiding this comment.
this.once calls this._on(type, listener, true) so I think this will recurse infinitely.
|
(the nits aren't important but I think the |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 074f52b in 17 seconds
More details
- Looked at
206lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_qUXWu6rDAmtXHJlv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b04063f in 10 seconds
More details
- Looked at
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. js-pkg/client/src/provider.ts:367
- Draft comment:
Theemitfunction usesdata: any = null, butYSweetEventdoes not includenull. Consider updating the type to includenullor change the default value.
protected emit(eventName: YSweetEvent, data: any = undefined): void {
- Reason this comment was not posted:
Confidence changes required:50%
Theemitfunction uses a default parameterdata: any = null, but theYSweetEventtype does not includenullas a possible value. This could lead to type inconsistencies.
Workflow ID: wflow_OtZknozfWhh8dBN7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
paulgb
added a commit
that referenced
this pull request
Dec 12, 2024
`y-websocket` has a `synced` flag, but it is a bit misleading because it only tells whether the initial handshake was completed. Knowing whether a document was synced _in general_ for `y-protocols` is a bit tricky, because we would need to compare the state vector and delete set. When using WebSockets, we can take advantage of the ordering guarantee and synchronous message processing in Y-Sweet to implement a very simple check of sync. The client stores two numbers, `lastSyncSent` and `lastSyncAcked`. When it sends an update to the server, it also increments `lastSyncSent` and then sends a separate `messageSyncStatus` message containing that number as the payload. When the server receives a `messageSyncStatus`, it simply echoes it verbatim. When the client receives the `messageSyncStatus` in return, it updates `lastSyncAcked` to the payload of the message. If `lastSyncAcked` = `lastSyncSent`, we know that all messages since the last update have been processed. Otherwise, there are outstanding changes. This builds on #352 and will enable #306. Demo: https://github.com/user-attachments/assets/60d4aec2-f025-4e84-a594-28fbc84c67cb
paulgb
added a commit
that referenced
this pull request
Dec 16, 2024
This builds on top of #352 to add offline support to the Y-Sweet provider. It's also exposed through the React bindings by passing `enableOfflineSupport={false,true}` to `YDocProvider`. The approach is similar to `y-indexeddb`, where updates are stored incrementally and then compacted when the set of updates gets large. Browsers generally store indexeddb in plain text on disk. We encrypt the data using `AES-GCM`, and store the key in a cookie. Since cookies often store credentials, they are often treated more sensitively than local storage (Chrome encrypts them on disk, and stores the decryption key in the OS's keychain; Firefox and Safari still use plaintext.) Each document gets its own "database" in indexeddb, but the key is shared across the origin. Since multiple tabs can have the same document open, they attempt to coordinate: - Before writing to the database, we attempt to fetch any updates we haven't seen and apply them - When a client updates, it broadcasts a message telling other clients to update
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is essentially a complete rewrite of
YSweetProvider. It incorporates the reconnect logic directly in the provider itself, instead of using the wrapper, which should make it easier to reason about. This paves the way to beginning to extend the protocol (as required for #306 and #308).