Issue with roslibjs and Webpack 5

[amosbyon1224]

I am using roslibjs for one of my projects that is currently trying to upgrade to Webpack 5 and I am running into some issues.

I've created a minimum reproducible project here:
https://github.com/amosbyon1224/roslib-webpack5

but the main issue seems to be with webworkify complaining about arguments being undefined.

roslibjs was working fine using webpack 4.27.1 but is now broken using webpack 5.6.0

[roddc]

any updates? How do we fix this issue?

[MatthijsBurgh]

I think the solution is in browserify/webworkify#43. Am I correct @amosbyon1224?

[jdekarske]

That repo hasn't PR'd in four years. Is this the best solution?

[lights0123]

Webworkify is 80 lines. It should be trivial to just copy it into this repo with the suggested fix.

[philippspindler]

Indeed, browserify/webworkify#43 fixes the problem.

I solved it by applying a local patch fix according to browserify/webworkify#43 via https://www.npmjs.com/package/patch-package to the webworkify dependency.

[MatthijsBurgh]

Please test the patch/webworkify branch.

Edit: CI is not happy: #462. Please help to fix this.

[MatthijsBurgh]

The CI works with the current webworkify release. Changing it with the suggested changes from browserify/webworkify#43 does brake the CI.

As the changes are that minimal, I don't see an option which works for both with the current setup of the library and webpack 5.

So I will close this issue for now. If anybody has any idea, drop it here and I will reopen it.

[maximeverreault]

@MatthijsBurgh the project could be bundled with Webpack 5 and its webworker implementation instead of Browserify with webworkify.

[maximeverreault]

I'm looking at the official documentation and the transport library "workersocket" (the one which breaks compatibility with webpack 5) is not even in the supported list of transport library. In fact, I can't even try to connect with this transport library using Typescript as the compiler won't let me set it to something unsupported.

Why is the support of this transport library is necessary if it's not even in the docs?

[MatthijsBurgh]

Moving to webpack will take significant time, which I don't have that much. So it will take a long time before it is there, unless other people could help.

So if you have experience with webpack, please start working on it

[maximeverreault]

I will give it a try. I've worked with Webpack 5 a couple of times, but not its specific web worker implementation. I will keep you updated

[MatthijsBurgh]

@maximeverreault Thanks!

[MatthijsBurgh]

@maximeverreault any progress?

[maximeverreault]

I successfully converted the project to a Webpack project, but not the web worker implementation. So I'm pretty much stuck. Meanwhile (I really need the compatibility to Webpack 5), I am creating a new roslib implementation using Webpack with Typescript

[maximeverreault]

If anyone is having this issue with Angular, here is the package I was taking about : https://www.npmjs.com/package/ngx-roslib

[trusktr]

I'm looking at the official documentation and the transport library "workersocket" (the one which breaks compatibility with webpack 5) is not even in the supported list of transport library. In fact, I can't even try to connect with this transport library using Typescript as the compiler won't let me set it to something unsupported.

Why is the support of this transport library is necessary if it's not even in the docs?

I've converted the codebase to ES Modules in #475 and it will be Webpack5-friendly after that is complete

For anyone curious, Webpack 5 throws errors on any file that does not strictly follow Node.js ESM rules. This makes the code much more friendly to native browser ESM, but not necessarily compatible out of the box without an import map and shim modules.

For example, after #475, importing the direct roslib source files won't work out of the box without a <script type="importmap"> tag to map node-style imports to modules that shim the exports via global variables. But with an import map and shims in place, roslib source files will be natively importable in browser ESM (and Webpack 5 will helpfully throw errors regarding anything that is likely to break that web compatibility like missing .js extensions).

[trusktr]

I successfully converted the project to a Webpack project, but not the web worker implementation. So I'm pretty much stuck. Meanwhile (I really need the compatibility to Webpack 5), I am creating a new roslib implementation using Webpack with Typescript

Speaking of TypeScript, I am planning to make a PR following #475 to convert it to ES class syntax, and then a PR after that to convert it to TypeScript (if maintainers here are open to using TypeScript).

My eventual goal is to compile it to WebAssembly with AssemblyScript (a TypeScript to WebAssembly compiler).

[trusktr]

I'm looking at the official documentation and the transport library "workersocket" (the one which breaks compatibility with webpack 5) is not even in the supported list of transport library. In fact, I can't even try to connect with this transport library using Typescript as the compiler won't let me set it to something unsupported.

I vote to delete WorkerSocket. Besides not being documented, and unless I overlooked something, it doesn't seem to do anything at all, and is adding complexity to the project for no gain.

For example, as we can see right here:

roslibjs/src/util/workerSocketImpl.js

Lines 6 to 16 in 15cf667

	function handleSocketMessage(ev) {
	var data = ev.data;
	if (data instanceof ArrayBuffer) {
	// binary message, transfer for speed
	self.postMessage(data, [data]);
	} else {
	// JSON message, copy string
	self.postMessage(data);
	}
	}

It appears that the only thing this code does is receive WebSocket messages for the purpose of sending them back to the main thread.

I myself have tried optimizations like these, and they were not fruitful in providing meaningful gains that outweighed their complexity. Network requests have never been a bottleneck in my cases. The bottleneck has always been with processing that is performed on data after receiving it (f.e. decompressing a JPG image, etc).

Once the data is back on the main thread, if it is PNG data that needs to be decoded, then that is where CPU time will be spent.

As we can further see in the next line, the SocketAdapter that contains the PNG decompression logic is instantiated on the main-thread, not in the worker, therefore decompression happens on the main-thread-side once data is sent back from the WorkerSocket's thread:

roslibjs/src/core/Ros.js

Line 82 in 15cf667

this.socket = assign(new WorkerSocket(url), socketAdapter(this));

A more beneficial feature would be a PNGDecoderWorker to do that heavy data processing in a worker rather than having a worker that spends a lot of time idling while waiting for a network request. (And once we compile this to WebAssembly, that PNGDecoderWorker will be even faster).

[mvollrath]

I vote to delete WorkerSocket. Besides not being documented, and unless I overlooked something, it doesn't seem to do anything at all, and is adding complexity to the project for no gain.

The purpose of WorkerSocket is decoupling the WebSocket handler from the main thread, so that WebSocket ACKs can be returned to the server as quickly as possible and offloading that backpressure to the client's Worker-to-main-thread leg. This is an optional mitigation and is even more relevant in ROS2 where the server backpressure OOM bug still exists. The WorkerSocket feature was implemented at the request of OSRF.

[mvollrath]

See the WorkerSocket PR for more background and motivation: #317

[trusktr]

Ah, thanks. I didn't know about that (I haven't blocked main thread enough for it to affect a backend yet).

[MatthijsBurgh]

I have experienced the issue myself for the first time.

I suggest that we use try...catch. Like this:

try {
    var work = require('webworkify');
} catch(ReferenceError) {
    var work = require('webworkify-webpack');
}

This would also require to add webworkify-webpack to the dependencies.

edit: We could also switch completely to webworkify-webpack if that works in all situations.

[MatthijsBurgh]

See #513 for the try...catch or #512 for the full migration.

[MatthijsBurgh]

Full migration showed to not work in situations without webpack. So #512 is closed.

[r0gi]

#513 works with webpack 5.67.0 (via create-react-app), thanks!