feat(node-api): create a node.js wrapper library for the JSON-RPC workspace protocol #3070

leops · 2022-08-16T16:30:24Z

Summary

This PR creates a new @rometools/backend-jsonrpc library that implements the Workspace API by wrapping the JSON-RPC protocol exposed by the daemon server. The library is split into a few modules that may be imported independently:

command resolves a file system path to the Rome binary
socket ensures the daemon process is spawned and opens a node.js Socket connection to the RPC channel
transport wraps an open socket and implements the JSON-RPC protocol as untyped method calls and responses
workspace is automatically generated from the JSON schema of the Workspace API, and wraps a Transport instance with an object implementing typed methods calls to the remote workspace

At the top level of the library a single createWorkspace method calls each of these in sequence, wrapping each layer with the next one and returning a Workspace object ready to use

Test Plan

I added a small test script that creates creates a workspace instance (using a local build of the binary from the Cargo target directory), executes a few method calls then shuts down

cloudflare-workers-and-pages · 2022-08-16T16:34:46Z

Deploying with Cloudflare Pages

Latest commit:	`1633ef9`
Status:	✅ Deploy successful!
Preview URL:	https://3ffddac4.tools-8rn.pages.dev
Branch Preview URL:	https://feature-node-api-jsonrpc.tools-8rn.pages.dev

View logs

github-actions · 2022-08-16T16:35:30Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	396	396	0
Failed	5550	5550	0
Panics	0	0	0
Coverage	6.66%	6.66%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12397	12397	0
Failed	3860	3860	0
Panics	0	0	0
Coverage	76.26%	76.26%	0.00%

github-actions · 2022-08-16T16:36:49Z

Playground for commit 7f2ecf5

ematipico

Is there any chance/way to make the Transport testable? I would like to test the error cases, to make sure it works as expected

ematipico · 2022-08-17T07:54:31Z

.github/workflows/pull_request.yml

@@ -81,6 +81,46 @@ jobs:
          command: test
          args: --workspace --verbose

+  test-node-api:


Not sure if this is the correct place. Although neither the workflow for JS file is the correct place. I mean, we should run these tests when we change Rust files AND JS files.

Maybe we should have a workflow just for this package?

npm/backend-jsonrpc/package.json

ematipico · 2022-08-17T07:57:43Z

npm/backend-jsonrpc/package.json

+    "JavaScript",
+    "TypeScript",
+    "format",
+    "lint",
+    "toolchain"


nit: are the keywords appropriate?

ematipico · 2022-08-17T07:58:06Z

npm/backend-jsonrpc/package.json

@@ -0,0 +1,28 @@
+{
+  "name": "@rometools/backend-jsonrpc",
+  "version": "0.8.0-next",


Probably we should start from 0.0.1? Or 0.0.0 if we don't intend to publish it?

This version number is the same as the main rome package and all the cli-* wrappers, but it gets updated automatically in the generate-packages.mjs script that runs during releases anyway so I could set it to 0.0.0 as a placeholder

npm/backend-jsonrpc/src/index.ts

npm/backend-jsonrpc/src/transport.ts

npm/backend-jsonrpc/src/workspace.ts

MichaReiser

In my view it would be useful to add documentation to all public types/methods and to add a small readme to the project as well (which also indicates, that this API is considered internal and may change at any point in time).

npm/backend-jsonrpc/src/index.ts

MichaReiser · 2022-08-17T12:51:33Z

npm/backend-jsonrpc/src/index.ts

+	await transport.request("initialize", {
+		capabilities: {},
+		client_info: {
+			name: "@rometools/backend-jsonrpc",
+			version: "0.8.0-next",
+		},
+	});
+
+	return wrapTransport(transport);


What happens if the transport fails to initialize a connection (or create a socket)? Does it throw any errors and if so which one?

Creating the socket isn't really up to the transport layer (it expects to be provided with an existing Socket instance on construction), but the createSocket function will throw an exception is the socket cannot be opened (the socket emits an instance of Error through the "error" event), and the remote call to initialize may also throw an RPC error object if the remote server reports an error

Would it make sense for us to introduce a RomeBackendError that is thrown in any situation where the backend runs into an error (and we use RomeError in higher level APIs?)

It could be useful to introduce a custom error class wrapping the RPC error objects since those contain structured informations (they may include a serialized RomeError for instance), but I'm not sure we can provide much value by wrapping the I/O errors coming from node itself because of an issue with the connection

MichaReiser · 2022-08-17T12:53:52Z

npm/backend-jsonrpc/tsconfig.json

+    /* Projects */
+    // "incremental": true,                              /* Save .tsbuildinfo files to allow for incremental compilation of projects. */
+    // "composite": true,                                /* Enable constraints that allow a TypeScript project to be used with project references. */
+    // "tsBuildInfoFile": "./.tsbuildinfo",              /* Specify the path to .tsbuildinfo incremental compilation file. */


I would prefer if we could delete all the setting that we aren't using. It makes it hard to see what our explicit settings are.

I'd prefer to keep them because we haven't figure out a correct build process, so if we need some specific property turned on, we have the benefit of the comments. We can clean it once the build process is final

MichaReiser · 2022-08-17T12:54:30Z

website/playground/build.rs

@@ -387,16 +68,10 @@ fn write_protocol() -> (String, Vec<String>) {
    // ensure it looks good by running it through the formatter
    let formatted = format_node(JsFormatContext::default(), module.syntax()).unwrap();


… server protocol

leops requested a review from ematipico as a code owner August 16, 2022 16:30

leops requested a review from a team August 16, 2022 16:30

leops temporarily deployed to aws August 16, 2022 16:30 Inactive

leops temporarily deployed to aws August 17, 2022 07:39 Inactive

leops temporarily deployed to aws August 17, 2022 08:39 Inactive

ematipico reviewed Aug 17, 2022

View reviewed changes

MichaReiser approved these changes Aug 17, 2022

View reviewed changes

leops temporarily deployed to aws August 17, 2022 14:32 Inactive

leops added 5 commits August 18, 2022 17:12

feat(node-api): create a node wrapper library for the JSON-RPC daemon…

932272a

… server protocol

add tests

d7f15ad

fix the test workflow

3e23ad8

force the tests to exit on completion

255fbcf

address PR review

cd00bbd

leops force-pushed the feature/node-api-jsonrpc branch from 9783183 to cd00bbd Compare August 18, 2022 15:13

ematipico approved these changes Aug 18, 2022

View reviewed changes

leops temporarily deployed to aws August 18, 2022 15:17 Inactive

update generated bindings

1633ef9

leops temporarily deployed to aws August 18, 2022 15:25 Inactive

ematipico merged commit 9626afa into main Aug 23, 2022

ematipico deleted the feature/node-api-jsonrpc branch August 23, 2022 07:47

		@@ -387,16 +68,10 @@ fn write_protocol() -> (String, Vec<String>) {
		// ensure it looks good by running it through the formatter
		let formatted = format_node(JsFormatContext::default(), module.syntax()).unwrap();

feat(node-api): create a node.js wrapper library for the JSON-RPC workspace protocol #3070

feat(node-api): create a node.js wrapper library for the JSON-RPC workspace protocol #3070

Uh oh!

Conversation

leops commented Aug 16, 2022 • edited by ematipico Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

cloudflare-workers-and-pages bot commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

github-actions bot commented Aug 16, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

github-actions bot commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leops commented Aug 16, 2022 •

edited by ematipico

Loading

cloudflare-workers-and-pages bot commented Aug 16, 2022 •

edited

Loading

github-actions bot commented Aug 16, 2022 •

edited

Loading

ematipico Aug 17, 2022 •

edited

Loading