Configurable hash #8596 #8603

SebCorbin · 2019-08-06T12:21:58Z

Refs #8596

Launch Checklist

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
tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

src/ui/map.js

asheemmamoowala

Thanks for this contribution @SebCorbin! I have just a few more changes 🙏

src/ui/map.js

src/ui/hash.js

test/unit/ui/hash.test.js

chloekraw · 2019-08-12T17:08:14Z

@SebCorbin thanks for this contribution! We're doing the branch cut for the next release tomorrow, so if you have any bandwidth to make the changes today, we can take another look and try to get it into v1.3

SebCorbin · 2019-08-12T17:23:15Z

@SebCorbin thanks for this contribution! We're doing the branch cut for the next release tomorrow, so if you have any bandwidth to make the changes today, we can take another look and try to get it into v1.3

@chloekraw Sure, all changes have been made, except for the last of @asheemmamoowala's comments which I either did not understand or is not required.

test/unit/ui/hash.test.js

ryanhamley · 2019-08-15T20:54:02Z

I'm seeing a couple issues while testing this out.

Step 1: If I have a page with hash: true, I start out in the expected hash configuration, e.g. http://localhost:9966/debug/#12.5/38.888/-77.01866

Step 2: But if I change the map to hash: 'foo' and reload the page, I get http://localhost:9966/debug/#12.5/38.888/-77.01866&foo=12.5/38.888/-77.01866 with the new hash just appended to the old one.

Step 3: Now if I go back to hash: true and reload, I get a bunch of errors. Invalid LngLat object: (NaN, 38.888) followed by Cannot read property '0' of undefined.

Likewise, if after step 2, I delete the entire hash and reload the page, I get the correct hash http://localhost:9966/debug#foo=12.5/38.888/-77.01866. However, if I then set the map back to hash: true and reload, I get http://localhost:9966/debug#NaN/NaN/NaN and the same errors as seen in step 3.

SebCorbin · 2019-08-15T21:42:43Z

@ryanhamley I would say this is fairly normal, with the current version if you put a wrongly formed hash, you would get a the same kind of error you get in Step 2 as there is currently no hash cleanup. As for the reload with another option value, we could at least drop the hash if the parsing fails, but we can’t just assume what would be a previous option, just ask and I’ll update the pull request to add the dropping mechanism.

asheemmamoowala · 2019-08-27T06:11:18Z

I'm seeing a couple issues while testing this out.

I would say this is fairly normal, with the current version if you put a wrongly formed

I agree with @SebCorbin , these issues seem related to work-in-progress code that is using the hash parameter, and not finished state of code.

Now if I go back to hash: true and reload, I get a bunch of errors. Invalid LngLat object: (NaN, 38.888) followed by Cannot read property '0' of undefined.

As for the reload with another option value, we could at least drop the hash if the parsing fails, ... just ask and I’ll update the pull request to add the dropping mechanism.

@ryanhamley Is the map usable after the errors in Step 3? If not, then @SebCorbin it could we good to add the dropping mechanism.

ryanhamley · 2019-08-27T18:47:12Z

Is the map usable after the errors in Step 3? If not, then @SebCorbin it could we good to add the dropping mechanism.

No, the map enters a broken state

SebCorbin · 2019-09-03T15:03:33Z

@ryanhamley @chloekraw PR updated 👋

ryanhamley

This is looking good. Thanks for sticking with it! I had a few suggestions but the overall functionality seems right to me.

ryanhamley · 2019-09-04T00:30:06Z

src/ui/hash.js


-    constructor() {
+    constructor(hashName: ?string) {
+        this._hashName = hashName;


We should probably do some validation here. If I set hash: 'hello world' or hash: 'hello\nworld', the hash is appended twice so it makes sense to ensure the hash is what we expect.

What do you recommend, display a warning/error or sanitize the hashName?

this._hashName = hashName && encodeURIComponent(hashName) should be all that's needed.

Ensure the hashName is defined and if so, encode it. The problem is that in _getCurrentHash, we're reading the hash out of the URI (const hash = window.location.hash.replace('#', '');) so it's already encoded. Then the check part[0] === this._hashName fails because we're comparing a pure string against an encoded one, i.e. 'hello%20world' === 'hello world'. So encoding the hashName from the get-go gives us an apples-to-apples comparison.

ryanhamley · 2019-09-04T00:32:29Z

src/ui/hash.js

        }

        if (bearing || pitch) hash += (`/${Math.round(bearing * 10) / 10}`);
        if (pitch) hash += (`/${Math.round(pitch)}`);
-        return hash;
+
+        if (this._hashName && !mapFeedback) {


Nit: can we just move this block into the else statement on line 74 to avoid checking mapFeedback a second time?

This is hardly possible, as we need bearing and pitch to be added to hash if needed. By the way, mapFeedback is not well documented so I don't understand if I need to take it into account when hashName is set.

It appears that you can remove the !mapFeedback part of this check entirely without causing any issues. I'm not entirely sure what the variable is for, but this is the only reference I can find for it in the code base and it looks like it's pretty much always true if there's a hash.

src/ui/hash.js

ryanhamley · 2019-09-04T00:49:15Z

src/ui/hash.js

+                return part;
+            }).filter(a => a);
+            if (!found) {
+                parts.push(`${this._hashName || ''}=${hash}`);


Do we need the || '' part here? If there's no hash name, then shouldn't we skip the = as well?

test-flow fails without this, see https://circleci.com/gh/mapbox/mapbox-gl-js/50268

Cannot coerce `this._hashName` to string because null or undefined [1] should not be coerced. src/ui/hash.js:92:31 92| parts.push(`${this._hashName}=${hash}`); ^^^^^^^^^^^^^^

We can get around this by assigning this._hashName to a variable at the top of the if statement and then changing the two references to this._hashName to the variable. That lets us get rid of the unnecessary and confusing || '' part of this while keeping Flow happy.

ryanhamley · 2019-09-04T00:51:59Z

src/ui/hash.js

+    }
+
+    _getCurrentHash() {
+        const hash = window.location.hash.replace('#', '');


Could you add a comment on this just to make it a little easier to grasp at a quick glance?

src/ui/map.js

test/unit/ui/hash.test.js

SebCorbin · 2019-09-10T19:18:09Z

@ryanhamley weekly reminder for this PR :)

ryanhamley · 2019-09-16T21:50:23Z

I promise I will review this in the next day or two @SebCorbin!

ryanhamley

Hi @SebCorbin thanks so much for your patience. I spent some time going over the code today and added a few responses to some of your questions and comments. I think it looks pretty good overall.

Here's what I see as the TODO list:

sanitize the hash name input
remove the !mapFeedback part of the if statement on line 81
remove the || '' and fix the Flow error on line 92
add a short comment for line 101

So just a few really quick changes and we'll be ready to merge! If you can make them by Monday or Tuesday, we should be able to get this into our next release.

ryanhamley · 2019-10-11T18:56:29Z

src/ui/hash.js


-    constructor() {
+    constructor(hashName: ?string) {
+        this._hashName = hashName;


this._hashName = hashName && encodeURIComponent(hashName) should be all that's needed.

Ensure the hashName is defined and if so, encode it. The problem is that in _getCurrentHash, we're reading the hash out of the URI (const hash = window.location.hash.replace('#', '');) so it's already encoded. Then the check part[0] === this._hashName fails because we're comparing a pure string against an encoded one, i.e. 'hello%20world' === 'hello world'. So encoding the hashName from the get-go gives us an apples-to-apples comparison.

ryanhamley · 2019-10-11T20:42:13Z

src/ui/hash.js

        }

        if (bearing || pitch) hash += (`/${Math.round(bearing * 10) / 10}`);
        if (pitch) hash += (`/${Math.round(pitch)}`);
-        return hash;
+
+        if (this._hashName && !mapFeedback) {


It appears that you can remove the !mapFeedback part of this check entirely without causing any issues. I'm not entirely sure what the variable is for, but this is the only reference I can find for it in the code base and it looks like it's pretty much always true if there's a hash.

ryanhamley · 2019-10-11T21:09:22Z

src/ui/hash.js

+                return part;
+            }).filter(a => a);
+            if (!found) {
+                parts.push(`${this._hashName || ''}=${hash}`);


We can get around this by assigning this._hashName to a variable at the top of the if statement and then changing the two references to this._hashName to the variable. That lets us get rid of the unnecessary and confusing || '' part of this while keeping Flow happy.

src/ui/hash.js

ryanhamley

🎉 Thanks for your patience @SebCorbin This looks great!

SebCorbin changed the title ~~Configurable hash (#8596)~~ Configurable hash #8596 Aug 6, 2019

Configurable hash (mapbox#8596)

d70a323

SebCorbin force-pushed the configurable-hash branch from 9977545 to d70a323 Compare August 6, 2019 12:30

Fix typo in doc

15c8213

andrewharvey reviewed Aug 7, 2019

View reviewed changes

src/ui/map.js Outdated Show resolved Hide resolved

Update jsDoc

c2f953f

asheemmamoowala suggested changes Aug 9, 2019

View reviewed changes

src/ui/map.js Outdated Show resolved Hide resolved

src/ui/hash.js Outdated Show resolved Hide resolved

test/unit/ui/hash.test.js Show resolved Hide resolved

Reviewed per @asheemmamoowala

e713cfa

asheemmamoowala approved these changes Aug 13, 2019

View reviewed changes

test/unit/ui/hash.test.js Show resolved Hide resolved

asheemmamoowala mentioned this pull request Aug 14, 2019

Add support for #map= hash prefix #7725

Closed

6 tasks

asheemmamoowala requested a review from ryanhamley August 15, 2019 00:15

Ensure hash is well formed

533dd54

ryanhamley suggested changes Sep 4, 2019

View reviewed changes

Review per @ryanhamley

b361f3a

SebCorbin force-pushed the configurable-hash branch from 276b677 to b361f3a Compare September 4, 2019 09:48

ryanhamley suggested changes Oct 11, 2019

View reviewed changes

Sanitize hash name input, fix flow and a bit of doc

5a1a710

ryanhamley approved these changes Oct 16, 2019

View reviewed changes

ryanhamley merged commit 6c7bd20 into mapbox:master Oct 16, 2019

SebCorbin deleted the configurable-hash branch October 22, 2019 07:45

andrewharvey mentioned this pull request Nov 6, 2019

Make hash configurable #8596

Closed

arindam1993 mentioned this pull request Nov 7, 2019

Performance Metrics Tracking #8931

Closed

7 tasks

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade mapbox-gl from 1.3.1 to 1.5.0 dyna-dot/iClient-JavaScript-s#9

Open

crock mentioned this pull request Jan 8, 2020

🐛 Corrected type for hash prop soal/vue-mapbox#161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable hash #8596 #8603

Configurable hash #8596 #8603

SebCorbin commented Aug 6, 2019 •

edited

Loading

asheemmamoowala left a comment

chloekraw commented Aug 12, 2019

SebCorbin commented Aug 12, 2019 •

edited

Loading

ryanhamley commented Aug 15, 2019

SebCorbin commented Aug 15, 2019 via email •

edited

Loading

asheemmamoowala commented Aug 27, 2019

ryanhamley commented Aug 27, 2019

SebCorbin commented Sep 3, 2019

ryanhamley left a comment

ryanhamley Sep 4, 2019

SebCorbin Sep 4, 2019

ryanhamley Oct 11, 2019

ryanhamley Sep 4, 2019

SebCorbin Sep 4, 2019

ryanhamley Oct 11, 2019

ryanhamley Sep 4, 2019

SebCorbin Sep 4, 2019

ryanhamley Oct 11, 2019

ryanhamley Sep 4, 2019

SebCorbin commented Sep 10, 2019

ryanhamley commented Sep 16, 2019

ryanhamley left a comment •

edited

Loading

ryanhamley Oct 11, 2019

ryanhamley Oct 11, 2019

ryanhamley Oct 11, 2019

ryanhamley left a comment

Configurable hash #8596 #8603

Configurable hash #8596 #8603

Conversation

SebCorbin commented Aug 6, 2019 • edited Loading

Launch Checklist

asheemmamoowala left a comment

Choose a reason for hiding this comment

chloekraw commented Aug 12, 2019

SebCorbin commented Aug 12, 2019 • edited Loading

ryanhamley commented Aug 15, 2019

SebCorbin commented Aug 15, 2019 via email • edited Loading

asheemmamoowala commented Aug 27, 2019

ryanhamley commented Aug 27, 2019

SebCorbin commented Sep 3, 2019

ryanhamley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebCorbin commented Sep 10, 2019

ryanhamley commented Sep 16, 2019

ryanhamley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley left a comment

Choose a reason for hiding this comment

SebCorbin commented Aug 6, 2019 •

edited

Loading

SebCorbin commented Aug 12, 2019 •

edited

Loading

SebCorbin commented Aug 15, 2019 via email •

edited

Loading

ryanhamley left a comment •

edited

Loading