Support for Deno & Bun (5 line change) #874
Conversation
|
The CI is failing, so review is on hold till CI is fixed |
|
@MatthijsBurgh thanks for specifying. It should pass the linter now. While an exception to the linter's globals check was maybe justified, it made me realize a more generalized approach for this PR. The revised version is smaller and adds support for Bun, Deno, and possibly other runtimes. I should talk to you directly about the other check that now passes (.ts extension). If I could avoid the change I would. Its in this PR because it's a functional difference on runtimes that follow the JS (ECMA Script) spec. Deno implements the browser (ECMA Script) specification with support for typescript. If the import statement is not an exact file path, it fails. |
deno.lock
Outdated
There was a problem hiding this comment.
I don't think you need to commit the Deno lockfile, since roslibjs is not shipping via Deno, right?
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| import { EventEmitter } from 'eventemitter3'; | |||
| import Ros from '../core/Ros.js'; | |||
| import { GoalStatus } from '../core/GoalStatus'; | |||
| import { GoalStatus } from '../core/GoalStatus.ts'; | |||
There was a problem hiding this comment.
This makes sense to me but could you double-check the resulting bundled code from Vite to make sure that this resolves okay for runtimes that don't support TypeScript natively like Deno and ts-node do? I assume Vite will handle this, but I'm not 100% certain since I use extensionless paths on my projects.
src/core/Ros.js
Outdated
| // Detect if in browser vs in NodeJS | ||
| if (typeof window !== 'undefined') { | ||
| // browsers, Deno, and Bun support WebSockets natively | ||
| if (typeof WebSocket == "function") { |
There was a problem hiding this comment.
Please fix the failing checks on this line to adhere to the project's style guidelines
|
I came to bump this, only to realize it was waiting on me! Sorry for the delay. Linter passes locally, its now up to date with the latest master, and I confirmed vite does bundle it correctly (screenshot) |
Public API Changes
None
Description
The only functional change is a conditional check for Deno (created by the author of NodeJS). Roslibjs effectively already supports Deno, because Deno very very closely implements web standards. The only thing causing it to fail is that Roslibjs assumes NodeJS when there's no window object.
The only other change (appending
.ts) is because Deno natively supports typescript, so long as the full file path is specified.E.g. the following now works: