Skip to content

Make mapbox access token configurable via layout attribute #729

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

Merged
merged 5 commits into from
Jul 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,10 @@ Plotly.plot = function(gd, data, layout, config) {
// so that the caller doesn't care which route we took
return Promise.all(gd._promises).then(function() {
return gd;
}, function() {
// clear the promise queue if one of them got rejected
Lib.log('Clearing previous rejected promises from queue.');
gd._promises = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels dangerous and like it will lead to impossible to debug situations... Should we not at least log a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'm fine with logging something.

Why it this dangerous though?

To add more info: I added ⏫ to ensure that a Plotly.relayout call that results in a rejected error can be followed by another Plotly.relayout without any extra work. Maybe the line ⏫ belongs in Plotly.relayout and in Plotly.restyle. Thoughts?

});
};

Expand Down
41 changes: 32 additions & 9 deletions src/plots/mapbox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,22 +47,21 @@ exports.layoutAttributes = require('./layout_attributes');
exports.supplyLayoutDefaults = require('./layout_defaults');

exports.plot = function plotMapbox(gd) {

if(!gd._context.mapboxAccessToken) {
throw new Error(constants.noAccessTokenErrorMsg);
}
else {
mapboxgl.accessToken = gd._context.mapboxAccessToken;
}

var fullLayout = gd._fullLayout,
calcData = gd.calcdata,
mapboxIds = Plots.getSubplotIds(fullLayout, 'mapbox');

var accessToken = findAccessToken(gd, mapboxIds);
mapboxgl.accessToken = accessToken;

for(var i = 0; i < mapboxIds.length; i++) {
var id = mapboxIds[i],
subplotCalcData = getSubplotCalcData(calcData, id),
mapbox = fullLayout[id]._subplot;
opts = fullLayout[id],
mapbox = opts._subplot;

// copy access token to fullLayout (to handle the context case)
opts.accesstoken = accessToken;

if(!mapbox) {
mapbox = createMapbox({
Expand Down Expand Up @@ -131,3 +130,27 @@ function getSubplotCalcData(calcData, id) {

return subplotCalcData;
}

function findAccessToken(gd, mapboxIds) {
var fullLayout = gd._fullLayout,
context = gd._context;

// first look for access token in context
var accessToken = context.mapboxAccessToken;

// allow mapbox layout options to override it
for(var i = 0; i < mapboxIds.length; i++) {
var opts = fullLayout[mapboxIds[i]];

if(opts.accesstoken) {
accessToken = opts.accesstoken;
break;
}
}

if(!accessToken) {
throw new Error(constants.noAccessTokenErrorMsg);
}

return accessToken;
}
12 changes: 12 additions & 0 deletions src/plots/mapbox/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ module.exports = {
}
},

accesstoken: {
valType: 'string',
noBlank: true,
strict: true,
role: 'info',
description: [
'Sets the mapbox access token to be used for this mapbox map.',
'Alternatively, the mapbox access token can be set in the',
'configuration options under `mapboxAccessToken`.'
].join(' ')
},
style: {
valType: 'string',
values: ['basic', 'streets', 'outdoors', 'light', 'dark', 'satellite', 'satellite-streets'],
Expand All @@ -55,6 +66,7 @@ module.exports = {
'Either input the defaults Mapbox names or the URL to a custom style.'
].join(' ')
},

center: {
lon: {
valType: 'number',
Expand Down
1 change: 1 addition & 0 deletions src/plots/mapbox/layout_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
};

function handleDefaults(containerIn, containerOut, coerce) {
coerce('accesstoken');
coerce('style');
coerce('center.lon');
coerce('center.lat');
Expand Down
17 changes: 15 additions & 2 deletions src/plots/mapbox/mapbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function Mapbox(opts) {

// state variables used to infer how and what to update
this.map = null;
this.accessToken = null;
this.styleUrl = null;
this.traceHash = {};
this.layerList = [];
Expand All @@ -57,7 +58,16 @@ proto.plot = function(calcData, fullLayout, promises) {
var self = this;

// feed in new mapbox options
self.opts = fullLayout[this.id];
var opts = self.opts = fullLayout[this.id];

// remove map and create a new map if access token has change
if(self.map && (opts.accesstoken !== self.accessToken)) {
self.map.remove();
self.map = null;
self.styleUrl = null;
self.traceHash = [];
self.layerList = {};
}

var promise;

Expand All @@ -83,6 +93,9 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) {
// mapbox doesn't have a way to get the current style URL; do it ourselves
var styleUrl = self.styleUrl = convertStyleUrl(opts.style);

// store access token associated with this map
self.accessToken = opts.accesstoken;

var map = self.map = new mapboxgl.Map({
container: self.div,

Expand Down Expand Up @@ -334,7 +347,7 @@ proto.updateLayers = function() {
};

proto.destroy = function() {
this.map.remove();
if(this.map) this.map.remove();
this.container.removeChild(this.div);
};

Expand Down
37 changes: 34 additions & 3 deletions test/jasmine/tests/mapbox_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var customMatchers = require('../assets/custom_matchers');

var MAPBOX_ACCESS_TOKEN = require('@build/credentials.json').MAPBOX_ACCESS_TOKEN;
var TRANSITION_DELAY = 500;
var MOUSE_DELAY = 100;

var noop = function() {};

Expand Down Expand Up @@ -187,6 +188,22 @@ describe('mapbox credentials', function() {
mapboxAccessToken: dummyToken
}).catch(function(err) {
expect(err).toEqual(new Error(constants.mapOnErrorMsg));
}).then(done);
});

it('should use access token in mapbox layout options if present', function(done) {
Plotly.plot(gd, [{
type: 'scattermapbox',
lon: [10, 20, 30],
lat: [10, 20, 30]
}], {
mapbox: {
accesstoken: MAPBOX_ACCESS_TOKEN
}
}, {
mapboxAccessToken: dummyToken
}).then(function() {
expect(gd._fullLayout.mapbox.accesstoken).toEqual(MAPBOX_ACCESS_TOKEN);
done();
});
});
Expand Down Expand Up @@ -475,6 +492,22 @@ describe('mapbox plots', function() {
});
});

it('should be able to update the access token', function(done) {
var promise = Plotly.relayout(gd, 'mapbox.accesstoken', 'wont-work');

promise.catch(function(err) {
expect(gd._fullLayout.mapbox.accesstoken).toEqual('wont-work');
expect(err).toEqual(new Error(constants.mapOnErrorMsg));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chriddyp Important Setting a wrong access token currently:

  • throws an error,
  • clears the map, and
  • does not update fullLayout.mapbox.accesstoken

Is this something you can work with the workspace? If not, please let me know. Alternatively, we could:

  1. as the above, but clear fullLayout.mapbox.accesstoken if it is invalid
  2. eat up the error ...
  3. anything else?

Note also that setting an invalid mapbox.style does essentially the same.

Copy link

Choose a reason for hiding this comment

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

@chriddyp @etpinard one thing we can do is to make our token validation impeccable (maybe validate our token against the real mapbox api endpoint). @BRONSOLO has opened an issue about it https://github.com/plotly/streambed/issues/7010. For now in my opinion when the user supplies a bad token/style we catch the error then pop up an error dialogue telling them what is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpan So, what are you proposing?

one thing we can do is to make our token validation impeccable

I'd vote 👎 for adding token validation in plotly.js. Making one request to validate the token and then one to plot the map sounds like an overkill. No?

For now in my opinion when the user supplies a bad token/style we catch the error then pop up an error dialogue telling them what is going on.

... as currently off this branch, correct?

Copy link

Choose a reason for hiding this comment

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

I'd vote 👎 for adding token validation in plotly.js. Making one request to validate the token and then one to plot the map sounds like an overkill. No?

No no no 😛 I meant when the user add a mapbox token in setting. our backend makes a request to the mapbox to validate the token. If it is not valid, they cant even add the token. So we are sure that all of the tokens in the the token dropdown are valid

Copy link

Choose a reason for hiding this comment

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

... as currently off this branch, correct?

yep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpan Thanks for the clarification!

});

promise.then(function() {
return Plotly.relayout(gd, 'mapbox.accesstoken', MAPBOX_ACCESS_TOKEN);
}).then(function() {
expect(gd._fullLayout.mapbox.accesstoken).toEqual(MAPBOX_ACCESS_TOKEN);
}).then(done);
});


it('should be able to update traces', function(done) {
function assertDataPts(lengths) {
var lines = getGeoJsonData(gd, 'lines'),
Expand Down Expand Up @@ -763,15 +796,13 @@ describe('mapbox plots', function() {
}

function _mouseEvent(type, pos, cb) {
var DELAY = 100;

return new Promise(function(resolve) {
mouseEvent(type, pos[0], pos[1]);

setTimeout(function() {
cb();
resolve();
}, DELAY);
}, MOUSE_DELAY);
});
}

Expand Down