docs: update Node.js example to run as expected #393

HipsterBrown · 2024-10-16T14:26:18Z

I'm very excited to be able to use the Viam TypeScript SDK for my Node.js projects! Thanks for all the hard work to make this integration possible @edaniels and @njooma! 👏

After running through the current documentation and example application, I ran into a few issues that I'm resolving with this PR.

HipsterBrown · 2024-10-16T14:26:51Z

Node.md

@@ -1,10 +1,10 @@
-# Node & Viam's TypeScript SDK
+# Node.js & Viam's TypeScript SDK


Using the official name "Node.js" for consistency across the documentation.

HipsterBrown · 2024-10-16T14:27:43Z

Node.md


 ## Requirements

-This document assumes you already have Node installed. If not, follow the [instructions](https://nodejs.org/en/learn/getting-started/how-to-install-nodejs) provided by Node.
+This document assumes you already have Node.js >= version 20 installed. If not, follow the [instructions](https://nodejs.org/en/learn/getting-started/how-to-install-nodejs) provided by Node.js.


The --env-file flag was introduced in v20, so calling this out just in case some folks are running v18 which is still under long term support.

HipsterBrown · 2024-10-16T14:32:11Z

Node.md


 ```ts
 // main.ts

-import wrtc = require('node-datachannel/polyfill');
+const wrtc = require('node-datachannel/polyfill');


Node.js supports two module formats: CommonJS (require) and ESM (import). While they can be mixed together in some programs, the expected syntax for each format is different. If we wanted to use ESM import statements it would look a little like reversed Python:

import wrtc from 'node-datachannel/polyfill';

However after updating the example code to use ESM, it appears the Viam TypeScript SDK does not have an ESM compatible module export for Node.js at the moment. So I've updated the example code to use CommonJS for now.

This file works fine for me in Node 20 mod your API key secret fix

import wrtc = require('node-datachannel/polyfill') works in Node 20? That's quite odd since it doesn't work in the REPL:

Could tsc be transpiling the code to be valid?

Yes tsc or tsx would transpile to js since node can't run typescript.

I believe eslint/the community in general prefers ESM style imports in TS? At least i often see lint errors when i do const foo = require('bar')

Node 22 can run TypeScript with the experimental type stripping flag and would fail with the example code as is. I think we should have valid JS in the examples, even if tsc papers over the syntax errors.

I believe eslint/the community in general prefers ESM style imports in TS? At least i often see lint errors when i do const foo = require('bar')

Yes, ESM is the official standard and should be used when possible. However, the current build output of the TS SDK cannot be imported as ESM in Node.js:

(The above example in the REPL is using the dynamic ESM import syntax but the same error applies when running main.ts with static ESM imports)

HipsterBrown · 2024-10-16T14:34:06Z

examples/node/package.json

@@ -17,7 +17,6 @@
    "ts-protoc-gen": "^0.14.0",
    "typescript": "^5.2.2"
  },
-  "type": "module",


This field enforces ESM for Node.js projects, which is not currently possible with the TS SDK, so removing this to run the project as CommonJS.

same for node 20

HipsterBrown · 2024-10-16T14:38:38Z

examples/node/src/main.ts

+const wrtc = require('node-datachannel/polyfill');
+const connectNode = require('@connectrpc/connect-node');
+
+// @ts-expect-error


This is required to pass the TypeScript compiler (tsc). If the project was updated to use something like tsx, it could run in a single command: tsx --env-file=.env src/main.ts

Otherwise, once Node.js v22 is more widely used, it could be run directly without the extra dependency using the type stripping feature: node --experimental-strip-types --env-file=.env src/main.ts

Even with tsx in place, I think it's still good to call this out for anyone who decides to copy-paste it into their own app that's not using tsx.

HipsterBrown · 2024-10-16T14:39:50Z

examples/node/src/main.ts

@@ -12,7 +14,7 @@ for (const key in wrtc) {
 async function connect() {
  const host = process.env.HOST;
  const apiKeyId = process.env.API_KEY_ID;
-  const apiKeySecret = process.env.API_KEY_SECRET;
+  const apiKeySecret = process.env.API_KEY;


The README uses API_KEY in the .env file instead of API_KEY_SECRET and this naming is more consistent across the other SDK documentation.

edaniels

This all looks good to me other than a discussion for main.ts (mod api key secret fix). 20 is about a year old and 18 is EOL in 6 months. I think it'd be better to be on 20.

HipsterBrown · 2024-10-16T16:52:55Z

I don't know why the E2E tests are failing. I ran them locally with the same result.

edaniels · 2024-10-16T17:33:38Z

@HipsterBrown maybe it's best to move to tsx instead of tsc while getting node --experimental-strip-types src/main.ts to work too? Whatever gives the most compat I'm all for

HipsterBrown · 2024-10-16T17:43:42Z

.gitignore

@@ -116,3 +116,5 @@ docs/dist
 src/api-version.ts

 *.DS_Store
+
+playwright-report/


Added this since I was running the playwright tests locally.

HipsterBrown · 2024-10-16T17:44:29Z

examples/node/package.json

  "scripts": {
    "preinstall": "make -C ../.. build",
    "test": "echo \"Error: no test specified\" && exit 1",
-    "start": "npx tsc && node --env-file=.env src/main.js"
+    "start": "tsx --env-file=.env src/main.ts"


Updated to use tsx, so no worry about waiting on the TypeScript compiler or leftover .js files.

HipsterBrown · 2024-10-16T17:45:39Z

examples/node/src/main.ts

+const wrtc = require('node-datachannel/polyfill');
+const connectNode = require('@connectrpc/connect-node');
+
+// @ts-expect-error


Even with tsx in place, I think it's still good to call this out for anyone who decides to copy-paste it into their own app that's not using tsx.

HipsterBrown · 2024-10-16T17:50:13Z

e2e/tests/connect-and-status.spec.ts

@@ -7,12 +7,12 @@ test('check resource names and multiple statuses', async ({ page }) => {
  const resourceNames = page
    .getByTestId('resource-names')
    .getByTestId('resource-name');
-  await expect(resourceNames).toHaveCount(4);
+  await expect(resourceNames).toHaveCount(3);


I'm not sure why the number of builtin resources is only 1 now, maybe something changed in the server implementation. It appears to be unrelated to my changes.

docs: update Node.js example to run as expected

4251afd

HipsterBrown requested a review from a team as a code owner October 16, 2024 14:26

HipsterBrown requested review from njooma and purplenicole730 October 16, 2024 14:26

HipsterBrown commented Oct 16, 2024

View reviewed changes

edaniels reviewed Oct 16, 2024

View reviewed changes

HipsterBrown added 2 commits October 16, 2024 13:42

chore(examples/node): add tsx for quicker start command

e2d7ca1

chore: add playwright-report to gitignore

f68f2f5

HipsterBrown commented Oct 16, 2024

View reviewed changes

HipsterBrown requested a review from edaniels October 16, 2024 17:45

test(e2e): update expected number of resources

297564e

HipsterBrown commented Oct 16, 2024

View reviewed changes

edaniels approved these changes Oct 16, 2024

View reviewed changes

njooma approved these changes Oct 16, 2024

View reviewed changes

HipsterBrown merged commit bc7385e into main Oct 16, 2024
3 checks passed

HipsterBrown deleted the nhehr/update-node-readme branch October 16, 2024 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: update Node.js example to run as expected #393

docs: update Node.js example to run as expected #393

HipsterBrown commented Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

edaniels Oct 16, 2024

HipsterBrown Oct 16, 2024

edaniels Oct 16, 2024

njooma Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

edaniels Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

edaniels Oct 16, 2024

edaniels left a comment

HipsterBrown commented Oct 16, 2024

edaniels commented Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

HipsterBrown Oct 16, 2024

		@@ -1,10 +1,10 @@
		# Node & Viam's TypeScript SDK
		# Node.js & Viam's TypeScript SDK

docs: update Node.js example to run as expected #393

docs: update Node.js example to run as expected #393

Conversation

HipsterBrown commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edaniels left a comment

Choose a reason for hiding this comment

HipsterBrown commented Oct 16, 2024

edaniels commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment