Ignore style.loadURL abort error when style is removed

[madoci]

Fixes #4400

Ignore the abort error raised by style.loadURL when the style is removed.

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.
• 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-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.40%. Comparing base (adc0495) to head (fc3cc98).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4425      +/-   ##
==========================================
- Coverage   87.94%   87.40%   -0.54%     
==========================================
  Files         246      246              
  Lines       33396    33397       +1     
  Branches     2173     2208      +35     
==========================================
- Hits        29371    29192     -179     
- Misses       3033     3191     +158     
- Partials      992     1014      +22     

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

[HarelM]

Thanks for taking the time to open this PR!
Looks like a good and clean solution, thanks!

[madoci]

Thank you @HarelM for the review. Should I open a new PR to add a test of the error firing in case it isn't an abort error, to ensure coverage of the if condition ? Or maybe do it here ?

[HarelM]

Here would be better. THANKS!

[HarelM]

Besides the test nit-picking this looks good.

[HarelM]

Nice!

[HarelM]

I see other places are using a simpler method:
expect((server.lastRequest as any).aborted).toBe(true);
I think it can be used here as well.
I'll open a PR to update the types so we can remove these "as any".

[madoci]

expect((server.lastRequest as any).aborted).toBe(true); is used to test if the request has been aborted, which the test above does ('cancels pending requests if removed'). In our case, we want to test that, when the request is aborted, no error are fired.

Removing these "as any" would be nice, thanks.

[HarelM]

Using expect((server.lastRequest as any).aborted).toBe(true); would remove the need to use this promise, and wait for it rejection. I find it more elegant. You might still need a sleep(0) though.

[HarelM]

I've updated the nise types, now you can use it without the "as any":
#4446

[madoci]

I've updated the nise types, now you can use it without the "as any"

Thanks !

Using expect((server.lastRequest as any).aborted).toBe(true); would remove the need to use this promise, and wait for it rejection. I find it more elegant. You might still need a sleep(0) though.

We would still need a sleep(0) since the rejection is asynchronous. Without it, the test succeed even if the Style fires the abort error. The promise is just a way to wait for the rejection we want to test rather than just wait for a bit with the sleep(0), but I agree it's not very elegant and it might be too bound to implementation. Should I revert to sleep(0) ?

[HarelM]

Yes please.