-
Notifications
You must be signed in to change notification settings - Fork 655
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
feat: Fix getPublicUrl to handle relative and absolute URLs #1472
base: master
Are you sure you want to change the base?
Conversation
Hi @acalcutt I hope you're doing well! Would you mind reviewing this PR when you have a moment? |
Seems like something is going on with the ci, which i don't think is related to this PR. I will have to figure that out before this can be approved. |
It does seem like this test is failing. I am not sure that port it is using is valid.
|
The test if failing because in the "setup.js" is "publicUrl: '/test/'" which due this PR is converted to absolute path. I propose to check for "contain" instead of "equal" in the test, and also I think it should be not breaking change for the users using this feature, hope so 🙏 |
What about the port that it is getting in that url, 37499. that does not seem correct since the test says it is running on 8888 Also, I am not sure if all urls that don't have publicurl should have a http url appended to it. some paths are meant to be local paths instead of http paths I think... |
if (publicUrl) { | ||
return publicUrl; | ||
try { | ||
return new URL(publicUrl).toString(); | ||
} catch { | ||
return new URL(publicUrl, getUrlObject(req)).toString(); | ||
} |
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.
What about the port that it is getting in that url, 37499. that does not seem correct since the test says it is running on 8888
Listening at http://[::]:8888/. it seems more like a client port than the server port.
The port is taken from the request object used in "getUrlObject" function which I didn't touch, just used as base of absolute path. Don't know how it gets there. There can be some port forwarding in pipline when running tests, don't know really.
Also, I am not sure if all urls that don't have publicurl should have a http url appended to it. some paths are meant to be local paths instead of http paths I think...
The functionality I have added only change how publicUrl is treated, to always create an absolute path and don't use relative paths for urls in styles, for me It makes sense to work like this, becase when I dont specify public url then the "getUrlObject(req).toString()" is used as default which wokrs as absolute path generated from request.
Also I have checked codebase where the getPublicUrl is used, and it is used only for styles, when getting all avaiable styles and then in "fixUrl" function which is used to fix urls like sprites and glyphs etc.. and also the function has the comment ("Replaces local:// URLs with public http(s):// URLs."). And there is no other place where the getPublicUrl is used. And when I use the --public_url with some abolute url like "/tileserver/" it doesnt make sense that I get this url for example ("sprite": "/tileserver/styles/basic-style/sprite", "glyphs": "/tileserver/fonts/{fontstack}/{range}.pbf"), because when I use it Plotly for example it tries to fetch data in relative path based on my current href... So in the end the --public_url is useless without defining the full absolute path. And also when getting the tiles from the data and public_url is relative it didnt work because the library that are used for preview doesnt work with relative urls (see mapbox/mapbox-gl-js#10407 (comment))
I would like to have an option to set some "prefix" like "/tileserver/" to all urls in styels. But if you dont like this change, I can try to do it in some more option way... to use some special character like "$/tileserver/" to detect that i want to use "return new URL(publicUrl, getUrlObject(req)).toString();" instead of basic "return publicUrl;"
WDYT ? 😅
0053c5c
to
85419ad
Compare
This PR updates the getPublicUrl function to handle relative publicUrl values. When a relative URL is provided, it is automatically converted to an absolute URL based on the request context. If an absolute URL is provided, it is returned as-is.
User story:
I encountered an issue while setting up an Nginx proxy with a prefix for the tileserver. The main problem was when I needed to specify the entire absolute URL using -u|--public_url (because when fetching .pbf tiles, the relative URLs didn't work). In scenarios where I don't know the domain the server will be used on, I needed a way to rely on absolute URLs. I believe this PR will help with this scenario, and I'm sure others may face a similar issue. 😄