-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Changes from all commits
f0f0247
85a8e7d
3f6e647
8a9f8aa
faf79b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() {}; | ||
|
||
|
@@ -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(); | ||
}); | ||
}); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chriddyp Important Setting a wrong access token currently:
Is this something you can work with the workspace? If not, please let me know. Alternatively, we could:
Note also that setting an invalid There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lpan So, what are you proposing?
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?
... as currently off this branch, correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yep There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'), | ||
|
@@ -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); | ||
}); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 anotherPlotly.relayout
without any extra work. Maybe the line ⏫ belongs inPlotly.relayout
and inPlotly.restyle
. Thoughts?