Skip to content

feat: async RequestTransformFunction#7184

Open
kikuomax wants to merge 14 commits intomaplibre:mainfrom
codemonger-io:async-transform-request
Open

feat: async RequestTransformFunction#7184
kikuomax wants to merge 14 commits intomaplibre:mainfrom
codemonger-io:async-transform-request

Conversation

@kikuomax
Copy link

@kikuomax kikuomax commented Feb 26, 2026

Proposed changes

  • Add Promise<RequestParameters> to the return type of RequestTransformFunction; i.e., it may be async.
  • Reorganize code where transformRequest is invoked so that no type errors are reported and existing unit tests won't break.
  • Introduce new unit tests that test async transformRequest. They are basically variants of existing unit tests involving transformRequest.

Related issue

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • N/A Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov
Copy link

codecov bot commented Feb 26, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.46%. Comparing base (fafc343) to head (5139b49).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/ui/map.ts 66.66% 2 Missing ⚠️
src/source/video_source.ts 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7184      +/-   ##
==========================================
- Coverage   92.71%   92.46%   -0.26%     
==========================================
  Files         289      289              
  Lines       24073    24074       +1     
  Branches     5108     5108              
==========================================
- Hits        22320    22259      -61     
- Misses       1753     1815      +62

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kikuomax kikuomax changed the title Introduce async RequestTransformFunction feat: async RequestTransformFunction Feb 26, 2026
@HarelM
Copy link
Collaborator

HarelM commented Feb 26, 2026

Thanks for taking the time to open this PR!
This looks a bit too complicated with all the instanceof...
Can these be reduced somehow? In async methods you can await a non promise returning method, so I think some of the code can be simplified, not sure about the rest of the code.
Maybe wrap the transfrom request in a promise to make it easier to work with? IDK...

@kikuomax
Copy link
Author

kikuomax commented Feb 27, 2026

@HarelM

This looks a bit too complicated with all the instanceof... Can these be reduced somehow? In async methods you can await a non promise returning method, so I think some of the code can be simplified, not sure about the rest of the code. Maybe wrap the transfrom request in a promise to make it easier to work with? IDK...

I did it intentionally not to break the existing unit tests. I first tried to simply prepend await to transformRequest calls, but doing so caused a lot of timeout errors in the unit tests. It seems that await introduces an additional event loop cycle whether the awaited value is a promise or not. I did not want to change the existing unit tests to make sure I did not break anything currently working and I ended up with the current solution.

@kikuomax
Copy link
Author

kikuomax commented Feb 27, 2026

@HarelM
I am working on reducing the instanceof Promise checks to simple await. A lot of exiting unit tests have to be modified, but I found it manageable. I am going to make this PR draft until I finish the modification. I will let you know when updates are completed.

@kikuomax kikuomax marked this pull request as draft February 27, 2026 16:33
@HarelM
Copy link
Collaborator

HarelM commented Feb 27, 2026

Thanks!

@kikuomax kikuomax marked this pull request as ready for review March 1, 2026 05:24
@kikuomax
Copy link
Author

kikuomax commented Mar 1, 2026 •
edited
Loading

@HarelM
I was able to replace all the instanceof Promise checks with simple await.

The only concern is that the extra microtask delay introduced by unconditionally awaiting transformRequest might cause subtle errors in some applications that, maybe unintentionally, depend on strict timing of the transformRequest resolution, as the unit tests broke.

HarelM
HarelM reviewed Mar 1, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 1, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 1, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 1, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 1, 2026
View reviewed changes
@HarelM
Copy link
Collaborator

HarelM commented Mar 1, 2026 •
edited
Loading

Looks a lot better now, thanks!
I've added some minor comments about delayedServerResponde and tests stuff.
There's also some map async code that I think is worth solving, but otherwise great work!

@kikuomax
Copy link
Author

kikuomax commented Mar 2, 2026

@HarelM Would mind me rebasing my branch to solve the conflict? Or would you prefer a merge commit?

@HarelM
Copy link
Collaborator

HarelM commented Mar 2, 2026

Whichever works for you, I'll squash merge it at the end...

kikuomax added 10 commits March 2, 2026 22:26
@kikuomax
feat: allow RequestTransformFunction to be async
d2d256f 
`RequestTransformFunction` may return a `Promise<RequestParameters>`.

Clients of `RequestManager` prepare for the `transformRequest` method
returning `Promise<RequestParameters>`. They check if the returned value
from `transformRequest` is a `Promise` before awaiting it so that they
do not disturbe the existing unit tests. Otherwise, we will get a lot of
timeout errors.
@kikuomax
test: test async transformRequest
9e0fffc 
Test all the modules that may call `transformRequest` execpt for
`video_source`.
@kikuomax
test: test async transformRequest in map_style.test
2224796 
The new test case tests an edge case where the style JSON is not
resolved by `Style` but by `Map`.
@kikuomax
style: apply lint --fix
da33f00
@kikuomax
docs: add record to CHANGELOG (async transformRequest)
7060971
@kikuomax
feat: reduce instanceof Promise checks to await
e63cb19 
Modify existing unit tests to deal with an extra microtask delay
introduced by unconditionally `await`ing `transformRequest`.

`Style.loadURL` becomes `async`. I think it should not harm because it
is not a public API and has no return value.

There are still `instanceof Promise` checks in `geojson_source`, because
it seems there is no simple workaround to the extra microtask delay.  I
guess we have to modify the logic in `geojson_source`, although, I have
not come up with any solution.

**Concern**:
The extra microtask delay might cause subtle errors in some applications
that depend on strict timing of the `transformRequest` resolution.

**Background**:
The `instanceof Promise` checks were introduced not to cause an extra
microtask delay when the `transformRequest` is synchronous and not to
break existing unit tests.
@kikuomax
feat: remove instanceof Promise checks from geojson_source
cfe6e2e 
The point of this commit is to delay the resolution of the result of
`_getLoadGeoJSONParameters` until the `_isUpdatingWorker` is set; i.e.,
inside `_dispatchWorkerUpdate`. Simply `await`ing the result of
`_getLoadGeoJSONParameters` immediately after the call of the function
breaks the behavior when `setData` or `setClusterOptions` are
consecutively called. It would cause a race condition.
@kikuomax
test: replace setTimeout with await sleep
8122fbf 
Because `setTimeout` cannot be awaited and might cause flaky tests.
`delayServerRespond` is no longer necessary.

Also remove redundant comments.
@kikuomax
refactor: replace Style.loadURL then/catch with await/catch
e84b94b
@kikuomax
test: fix geojson_source test case
ed5e758 
Fixes the test "modifying cluster properties with pending data" so that
it reflects the latest behavior. In the latest `main` branch, data and
cluster options are separatedly applied. Therefore, there are one more
`sendAsync` calls.
@kikuomax kikuomax force-pushed the async-transform-request branch from 1cff2be to ed5e758 Compare March 2, 2026 14:32
HarelM
HarelM reviewed Mar 2, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 2, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 2, 2026
View reviewed changes
HarelM
HarelM reviewed Mar 2, 2026
View reviewed changes
@HarelM
Copy link
Collaborator

HarelM commented Mar 2, 2026

I've added some comments after latest merge from main, overall looks good.
See that you take care of the remaining comments and I'll merge this.
THANKS!

kikuomax and others added 3 commits March 2, 2026 23:53
@kikuomax @HarelM
Update CHANGELOG.md
e0a4c1e 
Co-authored-by: Harel M <harel.mazor@gmail.com>
@kikuomax
test: refactor geojson_source.test.ts
ce5496b 
Move expects from `sendAsync` to the body of each test function.
Remove an unnecessary comment.
Revert an unnecessary change.
@kikuomax
refactor: replace Map._diffStyle then/catch with await/catch
e11685f
@HarelM HarelM enabled auto-merge (squash) March 2, 2026 15:55
auto-merge was automatically disabled March 2, 2026 15:56

Head branch was pushed to by a user without write access

@kikuomax
Copy link
Author

kikuomax commented Mar 2, 2026

@HarelM Sorry, I have pushed minor changes after your approval.

@HarelM
Copy link
Collaborator

HarelM commented Mar 2, 2026

No worries. Thanks for all the work on this! A great addition to this library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HarelM HarelM HarelM approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Async RequestTransformFunction

2 participants

@kikuomax @HarelM