[WIP] convert to ES Module format

[trusktr]

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: https://github.com/RobotWebTools/ros3djs/pull/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.

[trusktr]

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.

[MatthijsBurgh]

@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]

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]

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

[trusktr]

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]

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]

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]

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]

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]

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]

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]

I noticed a lot of time is spent building the roslibjsdocker container each time just to run the tests. We can publish this container image on docker and just re-use each time. That'll speed CI up quite a bit.

[trusktr]

@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?

[MatthijsBurgh]

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