Fix URL parsing in normalizeSpriteURL

[jcary741]

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.
• Write tests for all new functionality.
• Add an entry to CHANGELOG.md under the ## main section.

Related issues: #3897, #182, #4950
Related PRs: #3898, #3923, #645

This PR reverts #3898, which introduces issues with use cases where transformRequest() is used for URL signing (see #4950). It also disallows relative URLs for sprite URLs moving forward. However, since #182 and #3897 were opened, transformStyleFunction has been introduced (back in 2022), which can be used by developers to pre-transform relative URLs to whatever is most appropriate. Notably, this can support both 1) ArcGIS Vector Tile Style -style relative URLs at a separate domain than the map is being used from, and 2) relative URLs hosted at the same domain as the map. This PR adds examples for how to use transformStyle to make URLs absolute. I don't think it is exhaustive, but definitely covers the ArcGIS Vector Tile Style cases.

Also, normalizeSpriteURL() now also throws a detailed error message to guide developers how to solve the issue.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.12%. Comparing base (91e4945) to head (085a6a9).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4962      +/-   ##
==========================================
+ Coverage   88.95%   89.12%   +0.17%     
==========================================
  Files         269      269              
  Lines       38271    38272       +1     
  Branches     2369     2355      -14     
==========================================
+ Hits        34044    34111      +67     
+ Misses       3219     3148      -71     
- Partials     1008     1013       +5     

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

[jcary741]

Oh, do I need to re-build docs & commit, or does the CI do that somewhere?

[HarelM]

Thanks for taking the time to open this PR!
Is referring the previous PR the only solution to this issue?
Cc: @Kai-W

[jcary741]

It's hard to spot through the diffs, but this does preserve the usage of new URL() to parse URLs, which was introduced by #3898. But yes, mostly this is just adding documentation / an explicit error to direct developers to use transformStyle.

[jcary741]

Oh, just re-read your question and see I misunderstood. Yes, I think this is the only solution here. On the net, we remove one breaking change (the re-ordering of normalizeSpriteURL and transformRequest), for another (requiring URLs to be absolute, and directing developers to correct the issue as needed).

Half my time on this PR was just spent reading and cross-referencing the related issues / PRs and this is the absolute least invasive solution I came up with.

[Kai-W]

I see the problem and i didn't know signed URls were a thing.
Using TransformStyleFunction could work for most cases. I probably missed that function because it is part of setStyle not part of the constructor. In the docs the constructor only mentions transformRequest. TransformStyle is not and Option of the constructor. If this is the intended way of dealing with relative urls, then transformStyle sould also be added to the Constructor as well.

An other solution could be to add an additional transform request. I expected transformRequest to be able to handle relative URLs before they get validated and for the signing URLs usecase the URLS should be absolute, validated and the extension .json/.png should be appended. Calling transofrm request before normalizeSpriteURL with type "Sprite" and 2 times after the normalisation with "SpriteImage" & "SpriteJSON" could be an other Option. Shouldn't break existing code and gives the flexibilty to transform all Requests before the url gets Normalized.

[HarelM]

@Kai-W I'm not sure I fully understand what you suggest.
If you can this of a different way to solve this problem can you please elaborate or open a different PR to showcase the code?

[jcary741]

@Kai-W I considered that approach, but making multiple calls with the same URL could just as easily cause issues for developers who choose to implement non-idempotent callbacks for transformRequest. I do like your idea of adding transformStyle to the map constructor, and I think it would actually also make sense to have it persisted on the Style class instance (where right now it apparently must be passed with each setStyle call). I'd be happy to submit a separate PR if @HarelM agrees.

[HarelM]

Sure, you can create a different PR for style transform, I'll need to see the ergonomics around it to decide if it makes sense, and what would happen when you set both this transform and pass a function to setStyle, but I think it can be a good addition.

But let's close on this PR first. What is the current status? Can you guys agree on the needed solution?

[Kai-W]

@jcary741 solution works for me and an additionale PR for a persisted transformStyle sounds good as well

[HarelM]

This is not a breaking change to version 4, right? Just a breaking version between the pre-releases...?

[jcary741]

It's breaking between pre-releases, but not with version 4. It does make the requirement for sprite URLs to be absolute (post transformStyle) explicit, but I don't think that worked previously anyway based on #182.

[HarelM]

@Kai-W any objections to merge this?

[Kai-W]

no objections, works for me