-
Notifications
You must be signed in to change notification settings - Fork 48
docs: update Node.js example to run as expected #393
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
Changes from all commits
4251afd
e2d7ca1
f68f2f5
297564e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,3 +116,5 @@ docs/dist | |
src/api-version.ts | ||
|
||
*.DS_Store | ||
|
||
playwright-report/ | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,10 @@ | ||
# Node & Viam's TypeScript SDK | ||
# Node.js & Viam's TypeScript SDK | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the official name "Node.js" for consistency across the documentation. |
||
|
||
This document contains detailed instructions on including Viam in your Node project. For a runnable example, see the [examples directory](/examples/node/). | ||
This document contains detailed instructions on including Viam in your Node.js project. For a runnable example, see the [examples directory](/examples/node/). | ||
|
||
## 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
### Dependencies | ||
|
||
|
@@ -19,25 +19,25 @@ You can use either Yarn or NPM to install the dependencies. This document will u | |
|
||
### Polyfills | ||
|
||
Using the SDK with Node also requires the use of some polyfills. In your application's entrypoint (`main.ts`, `index.ts`, or something similar), you will need to register those polyfills: | ||
Using the SDK with Node.js also requires the use of some polyfills. In your application's entrypoint (`main.ts`, `index.ts`, or something similar), you will need to register those polyfills: | ||
|
||
```ts | ||
// main.ts | ||
|
||
import wrtc = require('node-datachannel/polyfill'); | ||
const wrtc = require('node-datachannel/polyfill'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Node.js supports two module formats: CommonJS ( 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file works fine for me in Node 20 mod your API key secret fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes tsc or tsx would transpile to js since node can't run typescript. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe eslint/the community in general prefers ESM style imports in TS? At least i often see lint errors when i do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||
for (const key in wrtc) { | ||
(global as any)[key] = (wrtc as any)[key]; | ||
} | ||
``` | ||
|
||
### Transport | ||
|
||
Communicating with your Viam machine in Node requires the use of a custom transport. In your app's entrypoint, you will also need to register the custom transport: | ||
Communicating with your Viam machine in Node.js requires the use of a custom transport. In your app's entrypoint, you will also need to register the custom transport: | ||
|
||
```ts | ||
// main.ts | ||
|
||
import connectNode = require('@connectrpc/connect-node'); | ||
const connectNode = require('@connectrpc/connect-node'); | ||
globalThis.VIAM = { | ||
GRPC_TRANSPORT_FACTORY: (opts: any) => | ||
connectNode.createGrpcTransport({ httpVersion: '2', ...opts }), | ||
|
@@ -51,9 +51,9 @@ To use the SDK, you can use similar instructions to those found on the [document | |
```ts | ||
// main.ts | ||
|
||
import VIAM = require('@viamrobotics/sdk'); | ||
import wrtc = require('node-datachannel/polyfill'); | ||
import connectNode = require('@connectrpc/connect-node'); | ||
const VIAM = require('@viamrobotics/sdk'); | ||
const wrtc = require('node-datachannel/polyfill'); | ||
const connectNode = require('@connectrpc/connect-node'); | ||
globalThis.VIAM = { | ||
GRPC_TRANSPORT_FACTORY: (opts: any) => | ||
connectNode.createGrpcTransport({ httpVersion: '2', ...opts }), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
await expect(resourceNames.getByText('base1')).toHaveCount(1); | ||
await expect(resourceNames.getByText('servo1')).toHaveCount(1); | ||
await expect(resourceNames.getByText('builtin')).toHaveCount(2); | ||
await expect(resourceNames.getByText('builtin')).toHaveCount(1); | ||
|
||
// 3 status iterations * 4 resource names (see main.ts for loop) | ||
// 3 status iterations * 3 resource names (see main.ts for loop) | ||
const statuses = page.getByTestId('statuses').getByTestId('status'); | ||
await expect(statuses).toHaveCount(12); | ||
await expect(statuses).toHaveCount(9); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this since I was running the playwright tests locally.