map-specific access token by peterqliu · Pull Request #8364 · mapbox/mapbox-gl-js

peterqliu · 2019-06-18T19:38:31Z

Launch Checklist

Adds an accessToken parameter to Map options, that optionally overwrites the token specified in mapboxgl.accessToken for just that map.

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page

peterqliu · 2019-06-18T19:46:28Z

        t.end();
    });

+    // t.test('map-specific token', (t) => {\


How can we instantiate a map with a bad token?

Would like to add a test for the positive case, where it starts with mapboxgl.accessToken = badtoken but gets overwritten by a valid token inside the map options.

How can we instantiate a map with a bad token?
The unit tests don't exercise any access token specific logic.

Map options are set-only with no ability to read back to options that were used to instantiate a map object. Since the access token is passed through to the RequestManager unit tests are probably suited to the behavior of that class.

asheemmamoowala · 2019-06-18T22:35:19Z

        t.end();
    });

+    // t.test('map-specific token', (t) => {\


How can we instantiate a map with a bad token?
The unit tests don't exercise any access token specific logic.

Map options are set-only with no ability to read back to options that were used to instantiate a map object. Since the access token is passed through to the RequestManager unit tests are probably suited to the behavior of that class.

peterqliu · 2019-06-21T20:27:03Z

bit stumped by the no-unused-vars lint error -- the flagged variable is used at https://github.com/mapbox/mapbox-gl-js/pull/8364/files#diff-3bdcadb35be47763a1abccc9707d22e0R375

asheemmamoowala

@ryanhamley @peterqliu IS there a reason to separate the implementation of the normalize...URL methods from the RequestManager? This PR would be a lot simpler if those were flattened, and all access to a token was through a RequestManager#accessToken() method.

ryanhamley · 2019-06-24T22:32:01Z

    },

    set baseApiUrl(url: string) {
+        console.log('setting');


console.log

ryanhamley · 2019-06-24T22:45:29Z

IS there a reason to separate the implementation of the normalize...URL methods from the RequestManager? This PR would be a lot simpler if those were flattened, and all access to a token was through a RequestManager#accessToken() method.

@asheemmamoowala I'm not sure I follow. the normalize...URL methods aren't being separated from RequestManager in this PR. Flattening them is hard because they have the same or similar signatures and it would be hard to know which method to call internally based on inputs. You could definitely have a RequestManager#accessToken() getter/setter but it would have to be an instance method which doesn't really help in getting the token in the normalize...URL methods.

asheemmamoowala · 2019-07-02T00:15:30Z

Flattening them is hard because they have the same or similar signatures and it would be hard to know which method to call internally based on inputs.

Why do the methods need to exist outside the RequestManager class? They are no longer exported, so the implementation could live within the RequestManager entirely.

You could definitely have a RequestManager#accessToken() getter/setter but it would have to be an instance method which doesn't really help ...

The RequestManager#accessToken getter will need to be public so that access token can be shared with Telemetry event types

asheemmamoowala

There is one instance of config.ACCESS_TOKEN in src/ui/attribution_control.js that needs to be updated to reference the map-specific access token as well.

ryanhamley · 2019-07-02T00:19:15Z

Ohhh I get what you're saying @asheemmamoowala. Yes, they could be moved into the RequestManager class. This PR didn't move them out of the class though. They just were never in it. Even if we moved the implementations into RequestManager, I'm not sure we could flatten them though.

peterqliu · 2019-07-03T23:56:49Z

sounds like something we could tackle in a future PR 👍 @asheemmamoowala @ryanhamley do you see any other blockers to this one?

ryanhamley · 2019-07-04T00:42:48Z

Agreed that we can refactor in a separate PR

Did you see Asheem's comment above @peterqliu?

There is one instance of config.ACCESS_TOKEN in src/ui/attribution_control.js that needs to be updated to reference the map-specific access token as well.

…specific-token

peterqliu · 2019-07-05T22:39:32Z

👍 done ^

ryanhamley

Looks good! 🎉

done :thumb:

peterqliu added 4 commits June 17, 2019 16:13

add map-specific token functionality

651b80d

add debug

f8278c9

test failing condition

975f6c7

documentation

1fa961f

peterqliu commented Jun 18, 2019

View reviewed changes

lint

3474c64

peterqliu marked this pull request as ready for review June 18, 2019 22:04

asheemmamoowala previously requested changes Jun 18, 2019

View reviewed changes

peterqliu added 4 commits June 18, 2019 18:43

stow custom access token in config

994c002

lint

5679b9b

wrap

0ddd9cd

stow local token in requestmanager

0bbffb3

ryanhamley reviewed Jun 21, 2019

View reviewed changes

Comment thread src/util/mapbox.js Outdated

peterqliu added 2 commits June 21, 2019 12:31

simplify custom token passing

aa2894a

flow typing

d89c2be

asheemmamoowala reviewed Jun 24, 2019

View reviewed changes

Comment thread src/util/mapbox.js

ryanhamley reviewed Jun 24, 2019

View reviewed changes

Comment thread src/index.js Outdated

},

set baseApiUrl(url: string) {

console.log('setting');

ryanhamley Jun 24, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

console.log

peterqliu added 6 commits June 28, 2019 14:50

alter conditional to account for no global token

d9f3480

lint

d974a32

fix lint

5f6e04f

wrap

861ce4a

restore conditional

975bfc3

pass lint

e857be0

asheemmamoowala reviewed Jul 2, 2019

View reviewed changes

peterqliu added 2 commits July 5, 2019 15:23

Merge branch 'master' of https://github.com/mapbox/mapbox-gl-js into …

6720934

…specific-token

custom tokens in attribution

3332331

ryanhamley approved these changes Jul 10, 2019

View reviewed changes

peterqliu merged commit 48af940 into master Jul 10, 2019

asheemmamoowala deleted the specific-token branch July 18, 2019 19:17

asheemmamoowala mentioned this pull request Jul 18, 2019

Allow for different Map instances to use different Mapbox access tokens #6331

Closed

peterqliu mentioned this pull request Aug 23, 2019

Auth font requests with custom token, when appropriate #8682

Open

etpinard mentioned this pull request Sep 27, 2019

Bump mapbox-gl to v1.3.2 plotly/plotly.js#4230

Merged

davidgilbertson mentioned this pull request Feb 9, 2020

@types/mapbox-gl: accessToken read-only? DefinitelyTyped/DefinitelyTyped#23467

Open

4 tasks

Uh oh!

Conversation

peterqliu commented Jun 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Launch Checklist

Uh oh!

peterqliu Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

asheemmamoowala Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asheemmamoowala Jun 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

peterqliu commented Jun 21, 2019

Uh oh!

asheemmamoowala left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanhamley Jun 24, 2019

Choose a reason for hiding this comment

Uh oh!

ryanhamley commented Jun 24, 2019

Uh oh!

asheemmamoowala commented Jul 2, 2019

Uh oh!

asheemmamoowala left a comment

Choose a reason for hiding this comment

Uh oh!

ryanhamley commented Jul 2, 2019

Uh oh!

peterqliu commented Jul 3, 2019

Uh oh!

ryanhamley commented Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterqliu commented Jul 5, 2019

Uh oh!

ryanhamley left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterqliu commented Jun 18, 2019 •

edited

Loading

ryanhamley commented Jul 4, 2019 •

edited

Loading