Skip to content
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

baseApiUrl w/ path & normalizeTileURL functionality #8466

Merged
merged 3 commits into from
Jul 18, 2019

Conversation

sumarlidason
Copy link

  • adds regex to trim tileURL prefix to /v4/...
  • adds filter to makeAPIURL preventing duplication of param 'access_token'
  • adds test for edge case

closes #8458

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

Arni Sumarlidason added 2 commits July 11, 2019 18:32
- adds regex to trim tileURL prefix to /v4/...
- adds filter to makeAPIURL preventing duplication of param 'access_token'
- reorders params on test case -> sku,access_token

closes #8458
@ansis ansis requested a review from ryanhamley July 17, 2019 18:31
Copy link
Contributor

@ryanhamley ryanhamley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just need to update the description of the test

test/unit/util/mapbox.test.js Outdated Show resolved Hide resolved
src/util/mapbox.js Show resolved Hide resolved
@sumarlidason
Copy link
Author

@ryanhamley I explained this a bit in the issue; when the TileHost metadata from the api returns with a path e.g. /mbx/, the URI passed to normalizeTileURL is not formatted into a mapbox namespace tilesURL: "mapbox://tiles/..." as you see here (from an api w/o a path):
Screen Shot 2019-07-18 at 10 07 31 AM

With a path the URI looks like this,
Screen Shot 2019-07-18 at 10 09 29 AM

You can see that w/ the logic prior to this PR the final URL is malformed w/ duplicate /mbx and /v4.

Another possible fix for this would be to adjust whatever parses this response's resp.tiles[0] into the mapbox namespace accounting for tile hosts w/ paths (mapboxgl.baseApiUrl):
image

I suspect that if the URI was properly coerced to the mapbox namespace this function would continue to function w/o modifications. I am not familiar enough with mapbox-gl-js to be aware of the ramifications -

@sumarlidason
Copy link
Author

@ryanhamley can you comment back and let me know what you think before me merge. 💯

@ryanhamley
Copy link
Contributor

Yep looks good!

@ryanhamley ryanhamley merged commit 69b3e18 into master Jul 18, 2019
@ryanhamley ryanhamley deleted the 8458_debugNormalizeTileURL branch July 18, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

baseApiUrl w/ path & normalizeTileURL functionality
2 participants