Canonicalize tile url #7594
Merged
Canonicalize tile url #7594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Here are the benchmark results. I didn't notice anything unusual, and really wouldn't expect to since the benchmarks don't actually request many tiles.
|
ryanhamley
reviewed
Nov 16, 2018
ryanhamley
reviewed
Nov 16, 2018
ryanhamley
reviewed
Nov 16, 2018
| for (let i = 0; i < params.length; i++) { | ||
| if (params[i].indexOf('access_token=tk.') === 0) { | ||
| params[i] = `access_token=${config.ACCESS_TOKEN || ''}`; | ||
| export const canonicalizeTileURL = function(url: string) { |
There was a problem hiding this comment.
This can be an arrow function I think. Same in line 135.
There was a problem hiding this comment.
following convention here.
There was a problem hiding this comment.
👍 I thought maybe that was the case. Good to know.
There was a problem hiding this comment.
Nit: I think the convention is export function fn(...), not export const fn = function(...). So this looks like a pre-existing omission.
There was a problem hiding this comment.
yeah I was following convention of the functions in this file – do you want me to change all of them ?
ChrisLoer
reviewed
Nov 16, 2018
ChrisLoer
reviewed
Nov 16, 2018
kkaefer
reviewed
Nov 19, 2018
kkaefer
reviewed
Nov 20, 2018
kkaefer
approved these changes
Nov 20, 2018
mollymerp
added
breaking change ⚠️
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
close #7400
This PR does a couple 🆕 things to improve parity between gl-native and gl-js:
config.API_URLmapbox://url, like the rest of the assets requested by the librarythis is technically changing existing behavior, but it shouldn't be breaking, unless a user was creating their own TileJSON endpoint to respond with an object where the
tilesurls were from a different domain than the TileJSON/other assets 😬 🙏 So I guess it maybe technically is breaking?Also, as far as I know the reason for providing multiple urls like
a.tiles.mapbox.com,b.tiles.mapbox.comwas to workaround the max open connections the browser will allow for any given origin for HTTP 1.0. When profiling locally on a large monitor, I do notice that some tiles requests are queued briefly (~10-50ms) when many tiles are being requested at once, but I'm not noticing a real big in time to first render (TTFR) or zoom/pan performance. @kkaefer any thoughts on how we should verify this?Launch Checklist