[WIP] convert to ES Module format #475

trusktr · 2021-09-12T03:12:32Z

Public Changes

None

Description

Converted to ES Modules (still ES5 classes though)

I'm doing this so that ros3d will be as close to fully ESM-compliant as possible (related PR in ros3d to convert to class syntax: RobotWebTools/ros3djs#438).

We may be able to complete that ros3d PR without this one (the source won't be as ESM-compatible, but the build output will be). We can complete both of these PRs without being 100% native ESM-compatible, but at least a lot closer (replacing all dependencies with ESM-ready versions would be the biggest step needed).

This is part of my longer term modernizing goal which includes the following in no particular order:

convert to TypeScript (need to convert roslib to classes first)
optionally compile to WebAssembly (with AssemblyScript, TS to Wasm compiler)
update Three.js to latest
replace unnecessarily big dependencies like EventEmitter2 (1600 lines of code for something that can be done in 100).

TODO

Run tests
Manually test Node.js usage
Manually test browser usage
Add shim module for bson in browsers
Restore WorkerSocket. I disabled some shims to get it to compile for browsers first. These need to be re-shimmed in the Rollup build.

Rayman

I like these changes. I've been thinking about starting this myself, but never got the time to do it

Rayman · 2021-09-14T13:56:34Z

src/RosLib.js


-module.exports = ROSLIB;
+export * from './core/index.js';


Do you need to hardcode .js for all imports? Is that mandatory?

It is mandatory by default in Node.js ESM, and very useful in browser ESM when using common types of web servers.

Webpack 5 now requires it by default too, to be compatible with official ESM spec and (most) browser applications (more on "most" below).

Some tools have historically automatically appended the extensions when performing lookup, but that has always been non-standard in the spec despite how standard it has been in build tooling.

The ES Module spec doesn't provide any sort of specification on file extensions, it only specifies that import specifiers are URLs, and the meaning of a URL depends on the server that receives that URL during a request.

In a browser, if your site is served with a regular average static file server, if you try to import './core/index' the browser will try to fetch the file 'current-file-folder/core/index' and not 'current-file-folder/core/index.js' (because to a URL the file extension has no particular meaning). Because of this, you'd need one of two options to make it work in most web apps:

a build step that adds the .js extensions to the source code, thus a regular static server (which is what most web apps have) will have no problem looking up the files

or use a special static server that automatically appends .js when performing file lookup that way source files do not need to have .js extensions

The vast majority of static file servers do not append .js extensions, and adding extra build steps is less desirable than not, so as you may now imagine writing .js extensions in the source files is the simplest way to be the most compatible everywhere.

…tput with Rollup instead of Browserify TODO: Some shims for Node.js were disabled to get this initially building for the browser.

trusktr · 2021-09-14T19:11:44Z

I just pushed an update that completes initial ESM format, and builds the output in three formats: ESM, CJS, and UMD.

The ./build/roslib.js and ./build/roslib.mni.js files changed to be UMD format, but the end result is the same for those two files thus they are (should be) backward compatible (needs testing).

The new build/roslib.cjs.js file is there in case anyone needs it for some reason (maybe they have an older version of Node.js that supports only CommonJS).

The new build/roslib.esm.js file is good for cases when someone needs to import the whole library using native ESM in a browser, where the browser (or more specifically the web app's server) will not know how to lookup Node-style import specifiers like import foo from 'foo'. The browser will try to fetch current-file-folder/foo which will not be found. So the single ESM bundle solves this case. Newer versions of Node.js should be able to import directly from the source files. Note that web apps can still import from source instead of from the ESM bundle via a couple options:

Modify dependencies to also be in ESM format, then use import-map.json to map Node-style specifiers to actual URLs
Use ESM CDNs like jspm, unpkg, or skypack to import dependencies from CDN URLs that automatically make available any packages from NPM (with best efforts at compiling any package that are still CommonJS to ESM). In many cases, this will works, but sometimes CommonJS format can be too dynamic for these CDNs to be able to convert some packages to ESM.

I haven't done any testing yet (adde TODOs to the OP).

This change should be considered a breaking change (because for example someone on an older version of Node without ESM will otherwise experience a break on their next npm install, etc), thus requires a major version bump.

trusktr · 2021-09-14T19:23:38Z

src/core/Ros.js

-var WorkerSocket = require('../util/workerSocket');
-var socketAdapter = require('./SocketAdapter.js');
+import WebSocket from 'ws';
+// import {WorkerSocket} from '../util/workerSocket.js'; // TODO


TODO I disabled some shims to get it to compile for browsers first. These need to be re-shimmed using Rollup.

Also another question I have is, is WorkerSocket actually useful? Has there been performance gains from this? I'm initially skeptical because fetching from network is a non-blocking operation. Does the data handling (from the browser engine, before finally passing that data to JS) have a big cost that the worker is putting on a separate thread?

Is there a benchmark somewhere? (I'm generally weary of complexity unless it really helps)

I think (but I'm not sure) that the CPU heavy work is decompressing the messages. Suppose you are sending a pointcloud with png based message compression. This work will then happen in a worker.

That would be good, but unless I overlooked it, it seems to me that only the WebSocket network requests run in the worker, and it passes the data back to the main thread to do whatever with it (like decompress the PNG). I don't see the decompressPng.js file being imported in the worker, I think I see it only postMessageing data.

mvollrath pointed out in issue 382 that #317 is why we need WebSocket in a worker.

trusktr · 2021-09-14T19:30:27Z

package.json

@@ -4,7 +4,9 @@
  "description": "The standard ROS Javascript Library",
  "version": "1.1.0",
  "license": "BSD-2-Clause",
+  "type": "module",


This is the main breaking change (major version bump)

MatthijsBurgh · 2021-09-14T19:50:15Z

@trusktr already thanks for your efforts.

Could you please enable GH actions for your repo. At the moment we only run on pushes, no PRs. But I think we should revert that.

trusktr · 2021-09-14T20:12:22Z

As far as I know, forked repositories automatically inherit GitHub Actions from their upstream repos (because they inherit the .yml files). GitHub added a new feature that requires owners of a repo to manually request Actions to be ran for pull requests. Do you see a button in the section below these comments? It is described here: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

EDIT: Ah, I see there's also the option to enable them for my fork.

trusktr · 2021-09-14T20:21:43Z

On my end I see it is waiting for actions to run:

trusktr · 2021-09-15T06:27:10Z

Alriiiiiight, tests are running (https://github.com/trusktr/roslibjs/runs/3606731719?check_suite_focus=true). Tomorrow is update-from-jshint-to-eslint day! It'll be nice to have the same tooling setup in both roslib and ros3d.

trusktr · 2021-09-16T09:51:15Z

eslint is working, build is working (but the output is not tested yet). I got rid of gruntify-eslint, and tried grunt-eslint but didn't use it; both were getting in the way so I simply used eslint CLI commands directly in the gruntfile shell section which just works with no issues. This leads to less complexity and maintenance (yay), with none of these sorts of changes I had to make without solving all the issue, with the end result being able to call eslint with grunt syntax rather than directly for no gain.

trusktr · 2021-09-16T10:33:08Z

I think the idea of being compatible with native browser ESM will have to wait, but at least the code will be essentially ES format with the import/export syntax, while the output build/roslib.esm.js file will still satisfy the native-browser-ESM case. There is a bit of extra work to do it: namely converting roslib's dependencies to ESM too.

Rayman · 2021-09-16T12:56:44Z

I would advise to keep the changes focussed on one topic as this makes it easier to review. Please create additional PRs for aditional topics (such as changing and fixing linting)

trusktr · 2021-09-17T06:31:13Z

I would advise to keep the changes focussed on one topic as this makes it easier to review. Please create additional PRs for aditional topics (such as changing and fixing linting)

That's what I thought initially, but the new syntax broke jshint, so I figured that rather than try to update a dated tool, I'd just put the new tool in place using the config we already have from ros3d.

After putting ESLint into place, there were very minimal code changes (just some missing semicolons, and a few unedfined vars, but all else stayed the same due to the lax rules that came from the ros3d repo).

I also thought about code formatting (f.e. with Prettier), but then that would cause changes everywhere and thus any change in develop unnecessarily conflict with this PR, so I definitely held off on that idea. I think it would be nice to do that later if you all are interested in that.

trusktr · 2021-09-17T06:32:02Z

I have mocha tests passing locally on my end now, just need a little more work on the karma side (this time shimming import in the browser as opposed to shimming require).

trusktr · 2021-09-17T06:33:18Z

src/core/Message.js


 /**
 * Message objects are used for publishing and subscribing to and from topics.
 *
 * @constructor
 * @param values - object matching the fields defined in the .msg definition file
 */
-function Message(values) {
+export function Message(values) {


As we can see in this file and others for example, even with the ESLint update, the main source code changes are the import statements, not much else changed.

trusktr · 2021-09-17T06:37:36Z

package.json

    "time-grunt": "^2.0.0"
  },
  "dependencies": {
    "cbor-js": "^0.1.0",
    "eventemitter2": "^6.4.0",
    "object-assign": "^4.0.0",
    "pngparse": "^2.0.0",
-    "socket.io": "^4.0.0",
+    "socket.io": "trusktr/socket.io#bf4bd150cff3c043c1f60ddf347e1b3ef0f1339a",


This is not ideal (see socketio/socket.io#4100). The alternative would be to update the socket.io import sites to import from a local file that shims the missing default function ((srv, opts) => new Server(srv, opts);) in Node.js, but in the browser just uses the io global.

TODO Make the custom shim file for socket.io so as not to have a custom version in package.json

trusktr · 2021-09-17T06:38:58Z

src/util/shim/socket.io.js

+
+function thro() {
+    throw new Error('transportLibrary "socket.io" is not supported in browsers. Set the `transportLibrary` option to "socket.io" only if you are use roslib in an environment like Node.js or Electron.')
+}


This file is totally wrong, I didn't realize socket.io works in a browser too.

TODO Update this so that it exports the global stuff from a global io variable from the global-script-tag version of socket.io.

trusktr · 2021-09-17T22:37:27Z

Alrighty, the base npm test (mocha and karma) tests pass. Now I need to do similar for the test-examples and test-tcp (and maybe test-workersocket if that actually matters, please confirm if it is actually running PNG decompression in a worker, or just merely making network requests and sending data back to main thread).

trusktr · 2021-09-17T22:38:56Z

test/karma.conf.cjs

+      // Installs cbor-js in a global CBOR variable so that our shim/cbor.js shim picks it up.
+      'node_modules/cbor-js/cbor.js',
+
+      {pattern: 'test/*.test.js', type: 'module'},


type: 'module' makes the scripts be loaded via native browser <script type="module">, and thus the passing tests are exercising the new roslib.esm.js output file in browser ESM.

trusktr · 2021-09-17T22:41:58Z

Gruntfile.cjs

    },
    karma: {
      options: {
        singleRun: true,
-        browsers: process.env.CI ? ['FirefoxHeadless'] : ['Firefox']
+        browsers: process.env.CI ? ['ChromeHeadless'] : ['Chrome']


I switched to Chrome because I was having issues getting the importmap polyfill working in Firefox:

SyntaxError: import not found, and no hint as to which file this happens in. guybedford/es-module-shims#186

request: clarity on installation in the README guybedford/es-module-shims#187

I decided to just get this all working natively first, then we can polyfill importmap in Firefox later if we still want to. But MS Edge is now built on Chromium, so at least the tests currently cover a large user base.

…e-lock.json is not useful for libraries (it does not get published with libraries), and having it reduces chances of us catching (during development or on CI) breaks in our library due to accidental in-range breaking changes in dependencies

* develop: Bump mocha from 9.1.1 to 9.1.2 (RobotWebTools#480)

trusktr · 2021-09-27T23:44:58Z

@MatthijsBurgh @Rayman I'm getting

rostopic list
ERROR: Unable to communicate with master!
Error: Process completed with exit code 1.

in the GitHub Action: https://github.com/trusktr/roslibjs/runs/3726509544?check_suite_focus=true

This doesn't seem to be related to my changes, as (I think) I have not touched anything related to the Docker ROS setup. I think that part should just work.

Any idea?

Sidenote, it says 1 workflow awaiting approval after the recent change to add PR runs. Do you want to run it here? Maybe it needs to run here for TOKEN to work and hence ROS stuff to work?

This allows people to install the package from a git repo (f.e. `npm install roslib@github-username/roslibjs`). by performing the `build` process in the `prepare` script. `prepare` is not only is executed before the `publish` script during publish (so that case is still satisfied), but `prepare` is also executed by `npm` during the `install` process. During `npm install` `devDependencies` are installed in a temporary folder, so those dependencies are even available to the `prepare` script. The end result is that someone can fork the repo, and easily install it, without having to publish the package under a new name, and without having to build and commit build artifacts into the repo (this is why the build artifacts were recently added to `.gitignore` in this repo a few commits back). People who rely on global variable script tags can still use URLs like `https://unpkg.com/browse/roslib@1.1.0/build/roslib.js` in their script tags src attributes, or just download such files to the app locally.

…s/roslibjs#475

(see reasoning in RobotWebTools/ros3djs@f56c58a)

MatthijsBurgh · 2021-11-18T11:51:34Z

@trusktr what is the status of this? I really appreciate your work and would like to merge this when ready.

…#475) Bumps [@rollup/plugin-node-resolve](https://github.com/rollup/plugins/tree/HEAD/packages/node-resolve) from 13.0.6 to 13.1.1. - [Release notes](https://github.com/rollup/plugins/releases) - [Changelog](https://github.com/rollup/plugins/blob/master/packages/node-resolve/CHANGELOG.md) - [Commits](https://github.com/rollup/plugins/commits/node-resolve-v13.1.1/packages/node-resolve) --- updated-dependencies: - dependency-name: "@rollup/plugin-node-resolve" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

initial conversion of files to ES Module format

e46aa82

trusktr mentioned this pull request Sep 12, 2021

[WIP] Convert to ESM and ES classes RobotWebTools/ros3djs#438

Closed

2 tasks

Rayman reviewed Sep 14, 2021

View reviewed changes

finish initial ESM format in source files, built ESM, CJS, and UMD ou…

2dc1db6

…tput with Rollup instead of Browserify TODO: Some shims for Node.js were disabled to get this initially building for the browser.

Merge branch 'develop' into convert-to-esm

8e71261

trusktr commented Sep 14, 2021

View reviewed changes

trusktr added 2 commits September 14, 2021 14:27

remove comment to hopefully trigger build

20385ad

make sure to call grunt with Gruntfile.cjs

a6ca753

trusktr mentioned this pull request Sep 16, 2021

Documentation on how to make roslibjs work with bson only mode on rosbridge #345

Open

switch to eslint, get lint and build passing (output not yet tested)

17d73e5

trusktr commented Sep 17, 2021

View reviewed changes

get mocha tests passing

31224af

trusktr commented Sep 17, 2021

View reviewed changes

trusktr added 2 commits September 17, 2021 09:31

set karma back to single run for CI

dcc10f5

get base karma tests passing (the ones ran by npm test)

aee7a57

trusktr commented Sep 17, 2021

View reviewed changes

trusktr added 2 commits September 20, 2021 18:07

initial conversion of test/examples to ESM

d3799a5

trusktr force-pushed the convert-to-esm branch from 179e506 to d3799a5 Compare September 21, 2021 01:08

trusktr added 7 commits September 27, 2021 12:32

add @babel/core that CI says was missing

54c6316

Merge branch 'develop' into convert-to-esm

ada3edf

* develop: Bump mocha from 9.1.1 to 9.1.2 (RobotWebTools#480)

update npm version in CI

17f9e7c

delete package-lock.json again (restored on accident from last merge)

793c940

install dependencies with --legacy-peer-deps, for now

bdc7f68

update workersocket test code to ESM

30fd409

update eslint to 8.0.0-rc.0

98edbfe

trusktr force-pushed the convert-to-esm branch 2 times, most recently from 908bee6 to eedbe41 Compare September 27, 2021 20:46

use nvm in Dockerfile to set the latest npm version

42d6590

trusktr force-pushed the convert-to-esm branch from 1c5a6b9 to 42d6590 Compare September 27, 2021 21:53

trusktr added 3 commits September 27, 2021 15:03

install chromium-browser for CI

bd5ed0c

build before testing

42b3004

the --no-sandbox flag for Chrome is needed when running in CI

1d428a1

trusktr added 2 commits September 27, 2021 16:45

add comment about DOckerfile CHROME_BIN env var

f3c43db

run non-ROS tests before doing anything with ROS

c8dda47

trusktr marked this pull request as ready for review September 28, 2021 00:00

trusktr marked this pull request as draft September 28, 2021 00:01

trusktr added 2 commits September 27, 2021 17:34

add a tet that ensures the code follows Node ESM spec

893751a

trusktr added a commit to trusktr/ros3djs that referenced this pull request Oct 1, 2021

use the ESM version of roslib from the WIP pull request, RobotWebTool…

a0fd39b

…s/roslibjs#475

trusktr added 2 commits October 3, 2021 13:30

ignore build output, libraries don't need lock files

2ce4ddc

(see reasoning in RobotWebTools/ros3djs@f56c58a)

simpify .npmignore

278f847

MatthijsBurgh closed this Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] convert to ES Module format #475

[WIP] convert to ES Module format #475

trusktr commented Sep 12, 2021 •

edited

Rayman left a comment

Rayman Sep 14, 2021

trusktr Sep 14, 2021 •

edited

trusktr commented Sep 14, 2021 •

edited

trusktr Sep 14, 2021 •

edited

Rayman Sep 15, 2021

trusktr Sep 17, 2021 •

edited

trusktr Sep 28, 2021

trusktr Sep 14, 2021

MatthijsBurgh commented Sep 14, 2021

trusktr commented Sep 14, 2021 •

edited

trusktr commented Sep 14, 2021

trusktr commented Sep 15, 2021

trusktr commented Sep 16, 2021 •

edited

trusktr commented Sep 16, 2021

Rayman commented Sep 16, 2021

trusktr commented Sep 17, 2021 •

edited

trusktr commented Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr commented Sep 17, 2021

trusktr Sep 17, 2021 •

edited

trusktr Sep 17, 2021

trusktr commented Sep 27, 2021 •

edited

MatthijsBurgh commented Nov 18, 2021

[WIP] convert to ES Module format #475

[WIP] convert to ES Module format #475

Conversation

trusktr commented Sep 12, 2021 • edited

TODO

Rayman left a comment

Choose a reason for hiding this comment

Rayman Sep 14, 2021

Choose a reason for hiding this comment

trusktr Sep 14, 2021 • edited

Choose a reason for hiding this comment

trusktr commented Sep 14, 2021 • edited

trusktr Sep 14, 2021 • edited

Choose a reason for hiding this comment

Rayman Sep 15, 2021

Choose a reason for hiding this comment

trusktr Sep 17, 2021 • edited

Choose a reason for hiding this comment

trusktr Sep 28, 2021

Choose a reason for hiding this comment

trusktr Sep 14, 2021

Choose a reason for hiding this comment

MatthijsBurgh commented Sep 14, 2021

trusktr commented Sep 14, 2021 • edited

trusktr commented Sep 14, 2021

trusktr commented Sep 15, 2021

trusktr commented Sep 16, 2021 • edited

trusktr commented Sep 16, 2021

Rayman commented Sep 16, 2021

trusktr commented Sep 17, 2021 • edited

trusktr commented Sep 17, 2021 • edited

trusktr Sep 17, 2021 • edited

Choose a reason for hiding this comment

trusktr Sep 17, 2021 • edited

Choose a reason for hiding this comment

trusktr Sep 17, 2021 • edited

Choose a reason for hiding this comment

trusktr commented Sep 17, 2021

trusktr Sep 17, 2021 • edited

Choose a reason for hiding this comment

trusktr Sep 17, 2021

Choose a reason for hiding this comment

trusktr commented Sep 27, 2021 • edited

MatthijsBurgh commented Nov 18, 2021

trusktr commented Sep 12, 2021 •

edited

trusktr Sep 14, 2021 •

edited

trusktr commented Sep 14, 2021 •

edited

trusktr Sep 14, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr commented Sep 14, 2021 •

edited

trusktr commented Sep 16, 2021 •

edited

trusktr commented Sep 17, 2021 •

edited

trusktr commented Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr Sep 17, 2021 •

edited

trusktr commented Sep 27, 2021 •

edited