Replace language server WebSocket connection with Ydoc #14438
+1,367
−183
Conversation
This file contains hidden or 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
4e6
added
the
CI: No changelog needed
4e6
force-pushed
the
wip/db/14142-ydoc-websocket
branch
2 times, most recently
from
7356274 to
4371781
Compare
| @@ -0,0 +1,3 @@ | |||
| module org.enso.ydoc.api { | |||
| exports org.enso.ydoc.api; | |||
Member
There was a problem hiding this comment.
- There is no need for a new API module.
- there already is YdocServerApi
- please put the new interfaces there as inner classes of
YdocServerApi.
Member
There was a problem hiding this comment.
- Possibly you can move
YdocServerApito the neworg.enso.ydoc.apimodule. - but the whole API needs to be co-located
| package org.enso.ydoc.api; | ||
|
|
||
| public class NoOpMessageCallbacks implements MessageCallbacks { | ||
| public static final NoOpMessageCallbacks INSTANCE = new NoOpMessageCallbacks(); |
Member
There was a problem hiding this comment.
- Please don't put
NoOpMessagesCallbacksimplementation class into the API - remove it
- if you have to have it, then encapsulate it
- put
public static final MessageCallbacks NO_OPintoMessageCallbacksand make the implementation class package private
Member
JaroslavTulach
left a comment
JaroslavTulach
left a comment
There was a problem hiding this comment.
- ideally
HostAccessis configured internally - and both tests and production code just work with the default configuration
- possible trick to achieve that is to crate a proxy around interfaces passed to JavaScript
| return this; | ||
| } | ||
|
|
||
| public Builder hostAccessBuilder(HostAccess.Builder hostAccessBuilder) { |
Member
There was a problem hiding this comment.
- Why is this a builder?
- Isn't just
HostAccessinstance good enough?
Suggested change
| public Builder hostAccessBuilder(HostAccess.Builder hostAccessBuilder) { | |
| public Builder hostAccess(HostAccess hostAccessBuilder) { |
- without
WebEnvironment.defaultHostAccessbeing public, one might need a builder - but having both builder and
WebEnvironment.defaultHostAccessseems unnecessary
| WebEnvironment.defaultHostAccess | ||
| // allowImplementations is required to call methods on JS objects from Java, i.e. to | ||
| // call methods on `YjsChannel` object returned from JS | ||
| .allowImplementations(YjsChannel.class) |
Member
There was a problem hiding this comment.
- allowing implementation of a well known interface is OK
- but opening up
allowAccessto a randomcallbacks.getClass()class is a security flaw - please create a proxy around
callbacksand expose its methods- preferrably via
@HostAccess.Explicit - or via
allowAccess(ProxyMessageCallbacks.getDeclararedMethod(""))but while hardcodingProxyMessageCallbacksclass
- preferrably via
| @@ -0,0 +1,3 @@ | |||
| module org.enso.ydoc.api { | |||
| exports org.enso.ydoc.api; | |||
Member
There was a problem hiding this comment.
- Possibly you can move
YdocServerApito the neworg.enso.ydoc.apimodule. - but the whole API needs to be co-located
4e6
force-pushed
the
wip/db/14142-ydoc-websocket
branch
from
71e9b86 to
8713e6f
Compare
| override def onMessage(message: Object): Unit = { | ||
| message match { | ||
| case m: String => | ||
| val webMessage = MessageHandler.WebMessage(m) |
Member
There was a problem hiding this comment.
- @4e6 you mentioned last week that the Java part of this PR was "suprisingly simple"
- the change looks really small
- it took me a while to locate where the essential Java part is
- if this
ServerCallbacksinYdocJsonRpcServeris all that's needed then I agree, yes, it seems surprisingly simple
| for (const item of items) { | ||
| // Only notify handlers if the message is from another sender | ||
| if (item.senderId !== this.senderId) { | ||
| this.notifyHandlers(item.payload) |
Member
There was a problem hiding this comment.
- As of 8713e6f we are probably dealing with ever growing
Y.Array - somehow we need to find out that the "message has been delivered" to all handlers
- that's kind of hard in a distributed framework
- where handlers can appear and diappear
- one solution would be to add
receiverIdtoitem- deliver the message only when
senderIdmatchesreceiverId - e.g.
if (item.receiverId === this.senderId)` - if there was only one receiver of a message
- then it'd be the receiver's responsibility to remove the
itemfromarray
- deliver the message only when
4e6
force-pushed
the
wip/db/14142-ydoc-websocket
branch
from
486cb1d to
c1ee0ea
Compare
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.
Pull Request Description
ydoc-servertalking directly to the engine+ls #14142Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.