Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More improvement to async Actor and replace more callbacks with promises #3269

Merged
merged 17 commits into from
Oct 25, 2023

Conversation

HarelM
Copy link
Collaborator

@HarelM HarelM commented Oct 24, 2023

I want to make sure that I don't mess things up so I'm keeping this PR open to see that the tests are passing.
I think I'll do most of the leftover improvements in this PR.
Let's see how big it gets.

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

Codecov Report

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

Comparison is base (fe8e847) 75.19% compared to head (4e704b5) 75.21%.

Additional details and impacted files
@@               Coverage Diff               @@
##           async-actor    #3269      +/-   ##
===============================================
+ Coverage        75.19%   75.21%   +0.02%     
===============================================
  Files              241      241              
  Lines            19318    19291      -27     
  Branches          4340     4326      -14     
===============================================
- Hits             14526    14510      -16     
+ Misses            4792     4781      -11     
Files Coverage Δ
src/render/glyph_atlas.ts 82.75% <100.00%> (ø)
src/render/image_atlas.ts 78.57% <100.00%> (ø)
src/render/image_manager.ts 46.19% <100.00%> (+0.31%) ⬆️
src/source/worker.ts 79.54% <100.00%> (ø)
src/util/actor.ts 80.61% <ø> (-0.20%) ⬇️
src/util/test/util.ts 92.53% <100.00%> (ø)
src/util/web_worker_transfer.ts 88.09% <0.00%> (ø)
src/source/worker_tile.ts 98.14% <97.40%> (+2.91%) ⬆️
src/source/vector_tile_source.ts 95.41% <88.00%> (-0.89%) ⬇️
src/render/glyph_manager.ts 92.07% <90.19%> (+0.81%) ⬆️
... and 1 more

... and 1 file with indirect coverage changes

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

@HarelM HarelM marked this pull request as ready for review October 24, 2023 20:29
@HarelM HarelM requested review from birkskyum and neodescis October 24, 2023 20:34
@HarelM
Copy link
Collaborator Author

HarelM commented Oct 24, 2023

This is now ready for review.
I've tested as much as possible, seems like things are working as expected.
The more I dig into this code the more weird things I find, and when I convert the code from callbacks to async functions I'm finding out a lot of hacks that were made a long the years, some of them I can't solve apparently.

CC: @smellyshovel @msbarry @ChrisLoer

This PR is against the actor async branch.
There is only one actor.send call left inside the ajax class, which will not part of this PR, I'm not even sure I would like to have it as part of this change in general as it might be too intrusive.

Most of the TODOs are done, there are a few left for the next days.
I hope this change will be able to be merged within a week or so.

@HarelM
Copy link
Collaborator Author

HarelM commented Oct 25, 2023

OK, I figured out the problem with the return promise, now fixed.
Aborting a request to a worker will create a pending promise, not sure what should be the right solution here, but for now I'm keeping the old behavior, when all the code is converted to promises it will be a lot easier to understand when something should return and what.
But this is out of scope of this PR.
This PR is now ready for review.

@HarelM HarelM changed the title Improve vector tile source More improvement to async Actor and replace more callbacks with promises Oct 25, 2023
Copy link
Collaborator

@neodescis neodescis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great changes. Keep them coming, @HarelM!

@HarelM HarelM merged commit 23db865 into async-actor Oct 25, 2023
4 of 12 checks passed
@HarelM HarelM deleted the async-actor-improved branch October 25, 2023 17:36
HarelM added a commit that referenced this pull request Nov 23, 2023
* Initial concept

* More improvements

* Fix lint

* Main commit to move most of the actor code to use promises and register. There are still things to do.

* Fix actor tests

* Remove more any when it comes to worker self typings.

* Improve more types, add more tests

* Improve more types, add more tests

* Fix lint

* Fix more tests

* Fix tests

* Fix some sytle tests

* Add comment for todo.

* Fix test related to message parameters change

* Fix remaining tests

* Added todo to accomodate for code review.

* Fix build test

* Fix geojson types

* Rename some events, remove clutter a bit

* Fix issue with multiple maps found in integration tests

* Fix tests and improve types and docs a bit more

* Final fixes to make the tests pass

* Fix build test

* a couple of tweaks (#3267)

* Add changelog item

* More improvement to async `Actor` and replace more callbacks with promises  (#3269)

* Improve vector tile source

* Fix unit tests

* Fix render tests for the case of 404

* Fix lint

* Remove unused method

* More improvements to remove callbacks and move to promises

* Fixed some tests, more fixes are needed.

* Fix remaining tests.

* Fix lint

* Fix tile parser code to pass render tests

* Fix lint

* Fix lint

* More fixes and tests in the infrasturcure when it comes to cancelling

* forgot to inclue this...

* Fix lint and build

* Convert parse to be a proper async method.

* Add more test coverate to actor, allow more testing in mock web worker.

* Replace getJson call with async code and remove more callbacks (#3273)

* Add abort between actors of main thread and worker thread with tests

* Fix lint

* Fix lint

* Fix node from lts to nvmrc

* Improve the code of vector tile and geojson workers

* Fix lint

* Add debug when tests are failing

* Do not throw in case of empty geojson tile

* Fix lint

* Move geojson worker source to be more async like.

* Change getJSON to be async extenally

* Update src/ui/map.test.ts

Co-authored-by: Bart Louwers <[email protected]>

---------

Co-authored-by: Bart Louwers <[email protected]>

* Move image queue to use promises, move getArrayBuffer to use promises (#3280)

* Migrate getArrayBuffer to be promise based

* Remove dead code in ajax.ts file

* Code review changes (#3278)

Co-authored-by: neodescis <[email protected]>

* Make image request return promises.

* Move process queue to the right location

* Increase build size

* Last fixes

* fix lint warning

* Fix lint

* update docs

Co-authored-by: neodescis <[email protected]>

* Update docs

Co-authored-by: neodescis <[email protected]>

* Fix docs

* Code review comments.

* Update src/util/image_request.ts

* Update src/util/util.ts

---------

Co-authored-by: neodescis <[email protected]>
Co-authored-by: neodescis <[email protected]>

* Remove TODOs, tidy up last things (#3301)

* Remove some TODOs

* Remove some todos, make ajax API fully promise based

* Move tileJson to be async

* fix lint

* Remove another TODO

* Move makexmlhttprequest to return a promise

* Make load_sprite async

* Added relevant tests

* Final fixes

* Update src/source/vector_tile_source.test.ts

Co-authored-by: neodescis <[email protected]>

---------

Co-authored-by: neodescis <[email protected]>

* Async actor docs additions (#3305)

* Fix missing classes in the docs

* Fixed the relevant docs in the places where API calls were changed.

* Fix lint

* Remove some "then" from tests

* remove todos, remove "then"s, simplify the ajax code a bit

* Remove as any from where it's not needed

* Fix code review expect stuff...

* explicit promises instead of array.

* Improve ajax further

* Improve docs for RTL plugin status and reduce code in geojson worker test

* Last simplification to actor, simplify a style test.

* Add missing docs comments

* Fix lint...

* Fix Actor XSS, introduce subscribe (#3329)

* Fix XSS, intorduce subscribe

* Update changelog

* Fix developer diagram with updated flow

* Async actor no log warnings and errors in unit tests (#3368)

* Upgrade jest, fix tests, remove console log messages.

* Fix incorrect additions, lint

* Async actor remove cancelables (#3371)

* Remove more places that use cancelable

* Remove more places with canceable

* Async actor callback and promise (#3374)

* Remove more places that use cancelable

* Remove more places with canceable

* callback and promise

* Improve sources code when it is related to callback

* Use callback failback when needed

* Update build size test

* Removed callbacks from various places, adding docs, simplifying tests

* Improve type

* Update src/util/actor.test.ts

Co-authored-by: neodescis <[email protected]>

* Make load async for all sources

---------

Co-authored-by: neodescis <[email protected]>

* Update changelog with most of the changes

---------

Co-authored-by: Michael Barry <[email protected]>
Co-authored-by: Bart Louwers <[email protected]>
Co-authored-by: neodescis <[email protected]>
Co-authored-by: neodescis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants