fix: order of normalizeSpriteURL and transformRequest in load sprites

[Kai-W]

This PR fixes the order of transformRequest and normalizeSpriteURL on loading Sprites.

(#3897)

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.

[HarelM]

Doesn't this break the current API of request transform, which is a public API?

[Kai-W]

Havent thought about that, could be. But if we call first transformRequest we dont have the inforamtion if it is an SpriteImage or SpriteJSON. And we cant first convert the urls to an Image/JSON because we need a absolute URL for normalizeSpriteURL to work

[HarelM]

I'm not saying this shouldn't be fixed, I just want to see if there's a way to do it without a breaking change.

[HarelM]

Otherwise, this will have to go into version 5 breaking change version, which can take a few month to be published.

[Kai-W]

I see some possible solutions but im not sure which is the best one:

• support relative urls than the order can stay this way. but this was rejected in other PR
• call transformRequest twice with the same url, once for Image and JSON. this doesnt change the enum but it is a silent api change because the url passed into transformRequest changes from the result of normalizeSpriteURL like http://localhost:9966/test/unit/assets/sprite1@2x.json with the pixel ratio and file extension to the raw url from the style: http://localhost:9966/test/unit/assets/sprite1
• rewrite normalizeSpriteURL that it can modify the url before transformRequest and do the validation afterwards. Im not sure if this is a good idea because theoretically the sprite url before the transformRequest doesn't need to be any valid url. Simply appending ${format}${extension} to the url can have other sideeffects i cant estimate. I'm still very new to maplibre-gl-js

[Kai-W]

Using the URL api to parse the spriteURl does work for absoulte URLs
For relative URLs we have to provide an additional base parameter to resolve against. The progres gets then resolved (base+relative)
This would be an replacement of the current implementation of absolute URLs but doesn't solve the Problem that relative URLs are parsed and validated before the transformRequest call.

The quick and dirty way:
To use the current system with differnt SpriteImage and SpriteJSON calls we need to extend the provided SpriteUrl with the information of ${format}${extension} before we call Transform.
We could try to modify the URL without parsing just based on String Operations.
This extension is always appended at the path of the URL.
If there are query Parameters thats directly before the first ? or without params at the end of the String
E.g. @2x.png should be inserted:
http://www.foo.com/bar?fresh=true -> http://www.foo.com/bar@2x.png?fresh=true
http://www.foo.com/bar -> http://www.foo.com/bar@2x.png
/bar -> /bar@2x.png
This doesn't need validation before the transformRequest.
This will work for absolute and relative urls.
It will break if its an absolute url and there is no path component
http://www.foo.com-> http://www.foo.com@2x.png
or sombody has arbitrary strings that get converted to urls in the transformRequest method
foobar -> foobar@2x.png

[Kai-W]

i tried implementing it in this branch. It works but with the drawbacks mentioned above
main...Kai-W:maplibre-gl-js:test
I still prefer the original solution with no modification of the SpriteURL before the call of transformRequest. It feels like a cleaner solution, but it has a small Public api change

[HarelM]

We can do public API changes as part of version 5, which we probably will target late this year or early next year.
If the normalize URL is only used in this code path then it makes sense to move it next to it.
Can you better clarify how the current method and the previous method are different? What won't work in the code you wrote (changing the normalizeUrl method)?

[Kai-W]

Current Method:
Parse URL with an regex in protocol, authority, path and params. If path is empty set to "/". Append ${format}${extension} to path and then reconstruct the url from the parts.
Afterwards call transformRequest twice with the modifyed urls (.josn + .png)
Works only with correct absolute URLs, otherwise parsing with the regex failes.
Works as well for absoulte URLs with no Path element e.g. http://www.foo.com -> http://www.foo.com/@2x.png/

New normalizeUrl:
Splits the String at the first "?" Assuming the Sting is an URL and a ? would seperate path and params of an URL
Then Appennd ${format}${extension} to the first part (path) and concatinate it back together. Then calling the transformRequest with both modifyed URLs
This works with absolute and relative URLs. But it wont work with arbitrary strings or absolute URLs with no Path. E.g
http://www.foo.com gets http://www.foo.com@2x.png/ is missing a "/" between .com and @2x
But i think thats an edgcase that would not happen in production. It would mean that there is no filename used for the sprites
Additionally with my current implemetation there is no validation of the URL returned by transformRequest.

New Api breaking Change:
First call transformRequest once withoput appending ${format}${extension} (.png/.json) to the Path.
Assume the result of transformRequest must be a valid absolute URL and then use normalizeSpriteURL to generate the .png/ .json urls based on the result of transfomrRequest.
Works with any sprite String.
Api change: transformRequest gets called once without .png/.json in the URL and one generic Sprite ResourceType
Additionaly we could change the normalizeSpriteURL to use the Browser URL API to remove the regex parsing.

An Option could be to merge the new normalize URL now to fix the bug and then merge the api breaking Change for Version 5.

[HarelM]

I think I'm OK with the last suggestion about merging this fix now and adding a breaking change feature to v5.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.04%. Comparing base (bec0505) to head (692217c).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3898      +/-   ##
==========================================
- Coverage   86.65%   86.04%   -0.62%     
==========================================
  Files         242      242              
  Lines       32479    32460      -19     
  Branches     1979     1980       +1     
==========================================
- Hits        28146    27930     -216     
- Misses       3391     3598     +207     
+ Partials      942      932      -10     

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