Async Actor

[HarelM]

The aim of this PR is to remove a lot of the callback hell that currently resides in the code.
It does it by exposing the low communication layer i.e. ajax and actor using the promise API.
Cancellation is done using AbortController.
In order to run a method in a worker or in the main thread there's a need to register to a specific event.

This will introduce a lot of breaking changes so it will be mandatory to create version 4 after this is merged.

Open tasks:

• Make sure API docs are correct
• Create a 3.6 version with optional promises I don't think it's worth the effort...
• Create a working example for add custom source too complicated for an example :-(
• Ask plugin owners that exist in the examples to make sure they can use this new code
• Think about other breaking changes related stuff.
• Present it in the monthly meeting
• Run benchmarks

Needed changes after this PR:

1. make style.ts more async await - Refactor RTL plugin loading code #3418
2. addSourceType should be for the global object - Move addSourceType to global object #3420
3. loadImage should return a promise instead of this - Remove callback from loadImage #3422
4. tileJson - improve any- Improve loadTileJson types #3423
5. Change addProtocol?

[codecov-commenter]

Codecov Report

Attention: 183 lines in your changes are missing coverage. Please review.

Comparison is base (684b314) 75.46% compared to head (78bb4a3) 75.50%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
src/util/ajax.ts	68.18%	28 Missing ⚠️
src/source/raster_dem_tile_source.ts	13.33%	26 Missing ⚠️
src/source/worker.ts	79.54%	18 Missing ⚠️
src/style/style.ts	66.66%	18 Missing ⚠️
src/source/geojson_worker_source.ts	72.88%	16 Missing ⚠️
src/source/raster_tile_source.ts	54.54%	15 Missing ⚠️
src/ui/map.ts	45.83%	13 Missing ⚠️
src/source/geojson_source.ts	71.79%	11 Missing ⚠️
src/source/video_source.ts	33.33%	10 Missing ⚠️
src/source/rtl_text_plugin.ts	76.92%	6 Missing ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3233      +/-   ##
==========================================
+ Coverage   75.46%   75.50%   +0.03%     
==========================================
  Files         241      241              
  Lines       19273    19230      -43     
  Branches     4344     4271      -73     
==========================================
- Hits        14545    14520      -25     
+ Misses       4728     4710      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[HarelM]

Cancellation can be achieved by passing an optional AbortController object to the method and firing a <cancel> message as done today I believe. More info can be found here: https://leanylabs.com/blog/cancel-promise-abortcontroller/
I guess that if this initial design is OK I can continue the experiment with a cancelable request.

[msbarry]

Cancellation can be achieved by passing an optional AbortController object to the method and firing a <cancel> message as done today I believe. More info can be found here: https://leanylabs.com/blog/cancel-promise-abortcontroller/

I guess that if this initial design is OK I can continue the experiment with a cancelable request.

That sounds good. In maplibre-contour I ended up returning a {cancel: function, value: promise} but I did not like how that turned out because all the code had to handle propagating the extra cancel function. It's much cleaner to pass in a cancelation hook to the arguments and just return a promise.

[HarelM]

I've updated the code to allow cancelation.
I've improved the typings a bit to be more correct, I hope.
There's not a lot of places where messages are being sent, I can use the new async way as part of this PR or a part of a different PR, let me know what will be easier to review.

[HarelM]

OK, I managed to figure out how to map between the request parameters and the response parameters and also how to properly register the handler with the relevant type safety.
Super helpful to find issues that way now.
I'm continuing to migrate places that send message to workers and moving them to async mode.
I'll push a commit once I finish with the geojson worker, removing callbacks makes the code a lot more easy to understand, it's a game changer!
I need to update all the tests to use the new actor, which is what prevents me from pushing a commit. I'll continue in the next night to push this forward, shouldn't be long I hope.
cc: @smellyshovel - as I know you looked into it.
Once this communication layer is without callback (to the outside) it should be easier to remove other callback parts I believe.

[HarelM]

I pushed the main commit for this PR just now.
I still need to make the test pass, lint, etc, but I wanted to push the current code I have as for my initial browser test it looks like it's working as expected to some extent.
It's getting big and I don't want it to get too big.
It's very nice to see that having the parameters typed well adds a lot of confidence between the worker and the main thread.
I also changed the Dispatcher class to register only the main thread messages while the worker is only registering the worker messages.
So now there's no need for this parent voodo stuff.
Overall, I think this is a good step forward.
There's still a few places that can benefit from the async which I haven't translated, not sure if I should do it as part of this PR or not...
I'll fix the tests and then decide how to proceed.

[msbarry]

So for "Ask plugin owners that exist in the examples to make sure they can use this new code" do you want us to just try running plugins against this build? Or is there a different way of "plugging in" that you think we'll need to switch to?

[HarelM]

The plugins will (hopefully) work as I haven't break anything yet in the add protocol API, but I do want to do a more deep API change (remove as much callbacks as possible from the code), and I need to know how well it will be received by plugin owners.
I also want to know if there's an interest in allowing to run part of the code in the worker in order to reduce ping-pong and if this is the case is it a good enough argument to "bite the bullet" of the API change in addProtocol.
If you can join today's monthly meeting it can be great as I'll present it there.

[msbarry]

OK got it. Maplibre-contour appears to work fine from this branch (npm start then go to http://localhost:9966/test/examples/contour-lines.html).

I also want to know if there's an interest in allowing to run part of the code in the worker

That would be great for maplibre-contour so it doesn't need to maintain its own worker. The custom source API is probably the right way to go right now, it will just take some work to get there because we need to expose some more maplibre internals on the main and worker thread for it to hook into.

Or we just incorporate the contour plugin directly in maplibre as a new layer type...

[HarelM]

I thought about making it a new layer, but then you lose the styling flexibility you have now.
I think the best approach might be to introduce it as a different source, this source can leverage a lot of the code that currently exist for both raster dem and vector worker and source.
Might be an interesting addition to v4 if you are up for it - but it requires a style-spec proposal to be approved first...
Having said that, this is not the only use case for a plugin, so I still need some input.
If I needed to write something that will run in the worker like a worker_source I will either need to copy the current worker source code or write something that is completely different.
I thought about a hook to a single method, like the addProtocol that you point to in the worker code that is executed instead of the fetch, this way you can keep it simple and powerful. IDK, still a work in progress...

[wipfli]

@msbarry is your life-of-a-tile document at https://github.com/maplibre/maplibre-gl-js/blob/main/developer-guides/life-of-a-tile.md still up-to-date after this?

[HarelM]

The changes are basically refactoring, meaning the code logic didn't change, so I believe it's still relevant, but I'll check nevertheless.

[msbarry]

There might be some references to "finish" or "done" functions which would now just be "continue method ..."

[HarelM]

I think I only saw the maybePrepare that needs a fix. feel free to update that and/or find other issues, I think it can be replaced with "wait for all" which is more general...

[HarelM]

I'm pushing the merge button! :-)