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

Preserve fresh=true query param #2702

Merged
merged 44 commits into from
Jun 14, 2016
Merged

Preserve fresh=true query param #2702

merged 44 commits into from
Jun 14, 2016

Conversation

xrwang
Copy link
Contributor

@xrwang xrwang commented Jun 9, 2016

For caching headers, allow fresh=true to be preserved and passed onto the http:// URL. This applies to mapbox:// only URLS.

fixes #2699

cc @scothis @jakepruitt @emilymcafee


function normalizeURL(url, pathPrefix, accessToken) {
function normalizeURL(parsedUrl, pathPrefix, accessToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this module be simpler if we parsed the url within normalizeURL instead of within its callers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mind parsing the same URL twice. This code isn't very hot.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's helpful to modify normalizeURL, it's already able to handle URLs with query strings. We'd only need to make sure that when URLs are manipulated before being passed to normalizeURL propagate the query string.

@jfirebaugh
Copy link
Contributor

If you do map.setStyle("mapbox://styles/username/id?fresh=true"), should the subsequent sprite, glyph, and tile requests that mapbox-gl-js makes also carry a fresh=true parameter?

@scothis
Copy link
Contributor

scothis commented Jun 9, 2016

If you do map.setStyle("mapbox://styles/username/id?fresh=true"), should the subsequent sprite, glyph, and tile requests that mapbox-gl-js makes also carry a fresh=true parameter?

The query string should be opaque to mapbox-gl-js. If the server handling the request thinks the tile, sprite or glyph URLs should contain the query param, it can add it before responding with the style.

@xrwang xrwang changed the title [not ready] Preserve fresh=true query param Preserve fresh=true query param Jun 10, 2016
@xrwang
Copy link
Contributor Author

xrwang commented Jun 10, 2016

tests passing, lmk how it looks! I'll squash down to one commit on merge.

formattedQuery = '?' + querystring.stringify(urlObject.query);
}

var parsedURL = url.format(urlObject.protocol + '/' + urlObject.pathname + formattedQuery);
Copy link
Contributor

@lucaswoj lucaswoj Jun 10, 2016

Choose a reason for hiding this comment

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

What's your intent using url.format here? As best I can tell, this method is supposed to take an Object argument, not a String argument.

@xrwang
Copy link
Contributor Author

xrwang commented Jun 14, 2016

\o/ thanks @lucaswoj. @scothis, I think this is ready to merge?

@lucaswoj
Copy link
Contributor

Hang on -- I'm seeing some rendering defects & error messages in the console when running this branch...

@lucaswoj
Copy link
Contributor

I think this is ready to 🚢 on 🍏

@lucaswoj lucaswoj merged commit 5366ddd into master Jun 14, 2016
@lucaswoj lucaswoj deleted the preserve-query-param branch June 14, 2016 18:13
davidtheclark pushed a commit that referenced this pull request Jun 15, 2016
* add a fresh query param on sprites and fonts urls

* lint

* lint2

* empty space

* parse urls

* update normalizeSourceUrl

* update notation

* change some name to avoid overlap with url lib

* make normalizeUrl normal again

* parse url for source and style

* clean up

* empty return

* spaces

* get rid of fresh false

* consistent naming, use Object.keys instead of getOwnPropertyNames

* use url parse throughout

* glyphs url too

* delete repeat

* get rid of url format

* add a fresh query param on sprites and fonts urls

* lint

* lint2

* empty space

* parse urls

* update normalizeSourceUrl

* update notation

* change some name to avoid overlap with url lib

* make normalizeUrl normal again

* parse url for source and style

* clean up

* empty return

* spaces

* get rid of fresh false

* consistent naming, use Object.keys instead of getOwnPropertyNames

* use url parse throughout

* glyphs url too

* delete repeat

* get rid of url format

* Refactor for clarity and robustness

* Fix normalization of composite urls
davidtheclark pushed a commit that referenced this pull request Jul 1, 2016
* add a fresh query param on sprites and fonts urls

* lint

* lint2

* empty space

* parse urls

* update normalizeSourceUrl

* update notation

* change some name to avoid overlap with url lib

* make normalizeUrl normal again

* parse url for source and style

* clean up

* empty return

* spaces

* get rid of fresh false

* consistent naming, use Object.keys instead of getOwnPropertyNames

* use url parse throughout

* glyphs url too

* delete repeat

* get rid of url format

* add a fresh query param on sprites and fonts urls

* lint

* lint2

* empty space

* parse urls

* update normalizeSourceUrl

* update notation

* change some name to avoid overlap with url lib

* make normalizeUrl normal again

* parse url for source and style

* clean up

* empty return

* spaces

* get rid of fresh false

* consistent naming, use Object.keys instead of getOwnPropertyNames

* use url parse throughout

* glyphs url too

* delete repeat

* get rid of url format

* Refactor for clarity and robustness

* Fix normalization of composite urls
@1ec5
Copy link
Contributor

1ec5 commented Jul 19, 2016

This appears to have been implemented independently for the native SDKs in mapbox/mapbox-gl-native#5554.

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.

Support query parameters in "mapbox://" urls
5 participants