Open Desplandis opened 1 year ago
Thanks for this thorough analysis ! :slightly_smiling_face:
Regarding the packaging, which is indeed the first the point to address, my opinion is that we should implement both solutions:
new URL(...) way
because it allows to address ESM modules users and it seems to be the way to go with web workers. (we can also check what are the options for mocha but if there are too complicated we can use the bundle created with the inlining way
)inlining way
because we still have users of the commonjs module (and maybe also for unit testing). From what I understand from the article you linked, the csp issue can be solved with the correct server configuration so documenting this extra server configuration needed for users of the commonjs module should be enough (it can be seen as the cost of not using ESM modules :slightly_smiling_face: )WDYT?
I would add that both ESM and CJS can be kept under the same umbrella but exposed the end-user through differents exports in the package.json
.
If there is a way to keep both solution and make that work through named exports and still keep compatibility for CJS users, it would be a win-win situation.
We finally decided to completely dropped support for CJS distribution (see proposal #2256). We'll push in a coming PR distribution as ES-only module (I already have a fully working branch). This could ease the use of workers for users with modern packers as well as fixing duplicates dependencies issues. Users of the itowns bundle will have their workers inlined by webpack (with worker-loader
).
BTW, I experimented with worker inlining with webpack + babel, however those scripts are a bit hacky and I don't want to add more junk into iTowns.
Since I'm not here on the coming week, I expect to push two PRs on those subjects (ESM distrib + Workers) the week of the 26th of February.
@Makaronelle Could I ping you to test some of those branches?
This issue is a technical discussion opened to all contributors and users of the library. Feel free to upvote (with ๐ ), comment, propose a solution and provide use-cases related to this issue.
Context
The Web Workers API provides a set of utilities to run a scripts in background threads. The worker thread can perform tasks without interfering with the user interface. Communication between a spawned worker and the main thread is done via bidirectional message passing.
In
iTowns
, we only have little CPU time between the rendering of two frames (~16.67ms). Any CPU-bound may block the main thread and by extension cause delays in event processing or painting. As a consequence, a user may find thatiTowns
feels sluggish in terms of framerate.One example of such slowness in
iTowns
is the visualization of large point-clouds. By profiling theenwine_simple_loader
example, we find that during event/painting delays (janks), 80% of CPU time is used by theLAZ
decompression procedure.This issue aims to discuss a correct way to integrate Web Workers in iTowns and identify parts of the library that may takes advantage of Web Workers (see Identified use-cases below).
The multiple Problems
First and foremost, Web Workers are a pain to integrate seamlessly for users let alone for library authors. A non exhaustive list of such problems (feel free to extend):
Promise
-based API (see Comlink for an easier way to work with workers).same-origin
as the parent context creating them (which excludes the use of CDNs).All those problems are exacerbated as a library as workers should work out-of-the-box with all bundlers. However...
The not-so-satisfying Solutions
The
new URL(...)
wayA recommended way to bundling external resources is to use the following syntax:
where
./worker.js
is a relative path known statically at bundle-time.Bundlers could then recognize this syntax and automatically bundle web workers. This is supported by all major bundlers: parcel, rollup, vite, webpack.
Note that the
import.meta.url
is only a valid syntax in the context of EcmaScriptESM
modules. However since we are currently transpiling and distributingiTowns
as CommonJS (CJS
) modules, this is not a valid syntax...I experimented with disabling transpilation to
CJS
modules and adding a"type": "module"
to ourpackage.json
but it breaks unit testing sincemocha
does not seems to support mockingESM
modules.The inlining way
Another way would be to inline the worker code as a string during the transpilation.
Transpiled code:
This would however necessitates to implement a pass in
babel
to pre-bundle the worker using a bundler (either webpack or rollup). Moreover, inlining worker may lead to Content Security Policy issues.Identified use-cases
EntwineLayer
)COPCLayer
)TLDR
We should not expect users of the library to encapsulate CPU-bound procedures (e.g. point cloud parsing) as the ESM standard does not provide an "easy" way to spawn a thread. However the lack of a standard way to bundle Web Workers leaves us with basically two choices with their own set of problems...