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

URL Transform callback as a Map option #5021

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

asheemmamoowala
Copy link
Contributor

@asheemmamoowala asheemmamoowala commented Jul 21, 2017

We have quite a few tickets (#4740, #2918, #4917, and gl-native/#7455) related to modifying XHR requests made by mapbox-gl-js for various resources. While these tickets are each slightly different, they all include an element of inspecting and modifying the XHR request before it is sent.
The common use cases break down to:

  • sending authentication headers/cookies with cross-origin requests
  • adding custom headers to requests
  • altering URLs dynamically

This PR addresses all these concerns by providing a per-map option for a transformRequest callback.

Example usage

let Map = new Map({    
  style: 'mapbox://styles/mapbox/streets-v9',
  transformRequest: (url, resourceType) => { 
    if (url.startsWith('http://myHost') {
      return {
        url: url.replace('http:', 'https:'),
        headers: { 'my-custom-header': true},
        withCredentials: true
    }
});

benchmark master 93a9922 4740-per-map-transform 0933fb7
map-load 160 ms 103 ms
style-load 91 ms 80 ms
buffer 937 ms 905 ms
fps 60 fps 60 fps
frame-duration 6 ms, 0% > 16ms 6 ms, 0% > 16ms
query-point 0.95 ms 0.98 ms
query-box 52.74 ms 53.28 ms
geojson-setdata-small 5 ms 4 ms
geojson-setdata-large 134 ms 131 ms

Checklist

// cc @anandthakker @jfirebaugh @kkaefer

@asheemmamoowala asheemmamoowala added GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform under development labels Jul 21, 2017
@asheemmamoowala asheemmamoowala changed the title [wip] Initial implementation of URL Transform callback as a Map option [wip] URL Transform callback as a Map option Jul 21, 2017
@asheemmamoowala asheemmamoowala force-pushed the 4740-per-map-transform branch 3 times, most recently from c2d6446 to 5cf7788 Compare July 24, 2017 22:16
@asheemmamoowala asheemmamoowala changed the title [wip] URL Transform callback as a Map option URL Transform callback as a Map option Jul 25, 2017
Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks @asheemmamoowala!

My main questions here are around the public API. See inline comments for specifics. Generally, I think this is a reasonable stopgap for custom sources, but we should replace it with custom sources when they're available, rather than maintaining two features with such substantial overlap.

@@ -24,7 +24,7 @@ module.exports = function run() {
const evented = new Evented();

const stylesheetURL = `https://api.mapbox.com/styles/v1/mapbox/streets-v9?access_token=${accessToken}`;
ajax.getJSON(stylesheetURL, (err, stylesheet) => {
ajax.getJSON({ url: stylesheetURL}, (err, stylesheet) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: either space after { and before }, or not -- not mixed (xN).

@@ -82,14 +82,24 @@ module.exports = function run() {
return evented;
};

class StubMap extends Evented {
constructor() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessary?

}

_transformRequest(url) {
return { url: url };
Copy link
Contributor

Choose a reason for hiding this comment

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

return { url };

module.exports = function(options, callback) {
module.exports = function(options, requestTransformFn, callback) {
if (!callback) {
callback = requestTransformFn;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make requestTransformFn a required argument.

@@ -126,7 +126,8 @@ function cached(data, callback) {
});
}

sinon.stub(ajax, 'getJSON').callsFake((url, callback) => {
sinon.stub(ajax, 'getJSON').callsFake((requestParameters, callback) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

({url}, callback) => { (x3)

@@ -107,7 +117,7 @@ function preloadAssets(stylesheet, callback) {
}

function getTile(url, callback) {
ajax.getArrayBuffer(url, (err, response) => {
ajax.getArrayBuffer({ url: url}, (err, response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

{url}

@@ -52,7 +52,7 @@ class RasterTileSource extends Evented implements Source {

load() {
this.fire('dataloading', {dataType: 'source'});
loadTileJSON(this._options, (err, tileJSON) => {
loadTileJSON(this._options, (url) => { return this.map._transformRequest(url, ajax.ResourceType.Source); }, (err, tileJSON) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

loadTileJSON(this._options, this.map._transformRequest, (err, tileJSON) => {

Let loadTileJSON supply ajax.ResourceType.Source.

@@ -7,7 +7,7 @@ import type {SerializedBucket} from '../data/bucket';
import type {SerializedFeatureIndex} from '../data/feature_index';
import type {SerializedCollisionTile} from '../symbol/collision_tile';
import type {SerializedStructArray} from '../util/struct_array';

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: keep the whitespace between imports and exports.

const EXTENT = require('../data/extent');
const resolveURL = require('../util/mapbox').resolveURL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for moving resolveURL? If it does move, it should go in something like util.js. mapbox.js is for Mapbox-specific URL munging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfirebaugh util.js already depends on window, so putting resolveURL in util.js creates a cyclic dependency. resolveURL could be moved to ajax.js, or left in geojson_source.js which was its only usage anyways

src/ui/map.js Outdated
* }
* });
*/
setRequestTransform(transform: Function) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Type transform more specifically than Function -- Function is like any: it turns off type checking.

Let's not make a setter for this, only a constructor option. This allows us to reduce internal coupling by passing a (normalized) transform function instead of the whole Map object. (If someone really needs dynamic behavior, they can put it inside the transform. But it's still a questionable idea: network requests may be triggered at any time, and we don't guarantee exactly when the transform gets called.)

Normalization looks like:

this._transformRequest = transform ?
    (url, type) => (transform(url, type || ajax.ResourceType.Unknown) || {url}) :
    (url) => ({url});

No need for _transformRequestCallback, and you can use map._transformRequest as a standalone function unconditionally from anywhere.

Also, is there a reason for the resource type to be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this suggestion. I didn't even consider the timing and order-of-execution issue with regards to Map#setRequestTransform.
reousrceType is optional for catching calls that don't provide a type.

@Yermo
Copy link

Yermo commented Jul 26, 2017

FWIW, the addition of this feature solves a big problem for me as I need to send an Authorization header with my requests. (I was on my way to implementing something like this, so thanks!)

src/util/ajax.js Outdated
export type RequestParameters = {
url: string,
headers?: Object,
withCredentials? : Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a bit more extensible, and consistent with the Fetch API (which we may eventually switch to internally):

    credentials?: 'same-origin' | 'include'

(It doesn't look like it's possible to emulate Fetch's 'omit' value using XHR, so I left that out.)

@@ -170,7 +170,7 @@ class GeoJSONSource extends Evented implements Source {
const options = util.extend({}, this.workerOptions);
const data = this._data;
if (typeof data === 'string') {
options.url = resolveURL(data);
options.request = this.map._transformRequest(resolveURL(data));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only place where the type isn't provided? Native uses the Source type here; let's do the same and then make this parameter required.

@@ -31,24 +31,26 @@ class ImageSprite extends Evented {
imgData: ?HTMLImageElement;
width: ?number;

constructor(base: string, eventedParent?: Evented) {
constructor(base: string, eventedParent?: Evented, transformRequestCallback?: Function) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the added parameter be required (in second position)?

@asheemmamoowala
Copy link
Contributor Author

asheemmamoowala commented Jul 27, 2017

Closes #4740 and closes #3874

@asheemmamoowala asheemmamoowala merged commit b528524 into master Jul 27, 2017
@asheemmamoowala asheemmamoowala deleted the 4740-per-map-transform branch July 27, 2017 22:10
@rayterrill
Copy link

Any idea when this will be released? This looks like it'll fix some of the CloudFront Signed Cookies issues we're currently experiencing.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 7, 2017

@rayterrill It'll be part of the next GL JS release. We're typically releasing new versions ~6 weeks. Mapbox GL JS v0.39 was released on July 21, so the next release will likely be end of August/beginning of September.

@rayterrill
Copy link

@kkaefer Thanks mate!

@volkergraf
Copy link

Just a Question .. the Transformation of the URL seems to work ok .. but now I'm getting a "Error: Unimplemented type: 3"

The Code looks like:

transformRequest: (url, resourceType) => {
              console.error(resourceType);
              alert("url: "+ url+"->"+ a_urls_to_blobs[url],);
              return {
                url: a_urls_to_blobs[url],
                headers: { "content-encoding" :"gzip"},
              }
            }

Do I have to set any type or something like that ?

@asheemmamoowala
Copy link
Contributor Author

@volkergraf can you provide more context on the error you are seeing ? A reproducible case with jsfiddle would be helpful in debugging.

@kkaefer
Copy link
Contributor

kkaefer commented Aug 8, 2017

It sounds like the response is zipped but not decompressed correctly. Why are you setting "content-encoding": "gzip" for the request header? GET requests like this one don't have a body, so it doesn't make sense to specify a content encoding. Browsers automatically specify an Accept-Encoding that includes gzip, so there's no header that you need to set to benefit from transport compression.

@volkergraf
Copy link

@kkaefer Well this was only a test, trying to get rid of the "Error: Unimplemented type: 3" which did not work =)

@volkergraf
Copy link

@asheemmamoowala Ok .. well the Scenario is the following:

We are developing an iOS Application using Apache-Cordova.

WebKit has a Problem concerning AJAX-Calls using **file://..-**Urls .. but there is a workaround, using the cordova-file-plugin and the WXWebKitView (with a modified Initializer), allowing you to kinda use file://...-Urls to access the deployd files (that will reside in www/foo/bar), but you can NOT use the MapBox-GL.js source URL like "...www/foo/bar{z}/{x]/{y}.pbf, you have to transform it to a blob:file://var......11-22-33-66-767-88/1122.pbf blob-URL.

Therefore I created an associative-Array, containing the Original-URLs that the mapbox-gl.js-map requests and it's corresponding blob:-URLs .. a_urls_to_blobs[url]

I can definitively confirm that the mapbox-gl.js CAN load the Tiles trough the blob-URLs .. but it seems that using transformRequest: might require a few more Parameters to return .. to make the whole Thing run .. at the Moment I am returning simply the "blob:"-URLs .. perhapes I might have to add a type or something like that .. any Ideas ?

@kkaefer
Copy link
Contributor

kkaefer commented Aug 8, 2017

Error: Unimplemented type: 3 means that PBF decoding failed. It likely fails because the file wsa not decompressed correctly because the headers aren't set. Mapbox GL itself doesn't decompress anything; it relies on the browser infrastructure to do so.

@rayterrill
Copy link

I integrated this and implemented the transformRequest credentials functionality today using CloudFront signed cookies - Everything appears to be working great! Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants