Skip to content

Use new OpenRPC mechanism to implement frontend comm #1986

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

Merged
merged 12 commits into from
Dec 23, 2023

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Dec 22, 2023

Follow-up to #1942.
Progress towards #265.
Also progress towards #1542 as we need to integrate the reverse frontend RPCs in the new comm infrastructure.

This ports the frontend comm to use the new infrastructure for generated comms and message types.

The frontend events were straightforward to port. They just required adding generation of event enums to the TS files. This enum is used to merge the various frontend event into a single message type tagged with the runtime ID, and then fan them back out at the handling sites.

The implementation of backend RPCs side-steps some more difficult questions about the role of callMethod() and its place in the positron API (issue upcoming).

  • Currently the comms are main-thread entities but the main users of callMethod() are extensions. There is no straightforward way of reusing the performRpc() implementation of PositronBaseComm from the extensions. So I've left a duplicated implementation inside positron-r.

  • It's not obvious whether the callMethod() API should be generic across runtimes, specific to runtimes, or a mix. I've left it unspecified for now. This is supported in generate-comms.ts by treating empty object specs with additionalProperties: true as a kind of "any" dictionary/struct. In Python this maps to JsonData and in Rust this maps to serde_json::Value. The parameter and result types are (un)specified this way.

With these changes I was able to remove the old event generation mechanism.

Before merging this we need to first merge the companion PRs for positron-python and amalthea.

yield '#[derive(Debug, Serialize, Deserialize, PartialEq)]\n';
yield '#[serde(rename_all = "camelCase")]\n';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation for using camelCase for object properties? If we like it better as a serialization format, it seems like it'd be better to just use it in all the openRPC contracts directly instead of using snake_case in the contracts and converting it to camelCase in all the generated code. (Not asking you to do that, just want to understand the change.)

Copy link
Contributor Author

@lionel- lionel- Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh hmm I think this should be:

#[serde(rename_all(serialize = "camelCase"))]

That's because we generate camelCase for TS and snake_case for Rust so serialisation should rename the fields to match the casing. This is what we have been doing before and still makes sense to do?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use camelCase in TS for object properties -- they're still snake case since that's the wire format right now.

/**
* The MIME type of the plot data
*/
mime_type: string;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just done that (with corresponding posit-dev/ark@8feb3f0 commit) but let me know if you'd like to take another approach here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry our messages crossed.

I see, there was a bug in the generation of some TS interfaces where we still used camelCase. Fixed in 234449b!

@lionel- lionel- force-pushed the feature/frontend-comm branch from 5b6a37e to 234449b Compare December 23, 2023 09:46
@lionel- lionel- merged commit bc94fec into main Dec 23, 2023
@lionel- lionel- deleted the feature/frontend-comm branch December 23, 2023 10:39
dfalty pushed a commit that referenced this pull request Jan 2, 2024
Use new OpenRPC mechanism to implement frontend comm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants