-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
fix: order of normalizeSpriteURL and transformRequest in load sprites #3898
base: main
Are you sure you want to change the base?
Conversation
@@ -7,8 +7,7 @@ export const enum ResourceType { | |||
Glyphs = 'Glyphs', | |||
Image = 'Image', | |||
Source = 'Source', | |||
SpriteImage = 'SpriteImage', | |||
SpriteJSON = 'SpriteJSON', | |||
Sprite = 'Sprite', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this break the current API of request transform, which is a public API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, this will have to go into version 5 breaking change version, which can take a few month to be published.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/[email protected] 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/[email protected]?fresh=true
http://www.foo.com/bar -> http://www.foo.com/[email protected]
/bar -> /[email protected]
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://[email protected]
or sombody has arbitrary strings that get converted to urls in the transformRequest method
foobar -> [email protected]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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://[email protected]/ 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm OK with the last suggestion about merging this fix now and adding a breaking change feature to v5.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
This PR fixes the order of transformRequest and normalizeSpriteURL on loading Sprites.
(#3897)
Launch Checklist
CHANGELOG.md
under the## main
section.