-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor!: UrlSearchParams based Hash
#7073
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
base: main
Are you sure you want to change the base?
Changes from 21 commits
18ab1a8
bd37fe2
53977e2
9a4aa80
661f343
80f0973
578887c
2cc15ba
be00422
a3fa7b6
615b59c
919d058
38a0c92
f59809a
85dcd78
9ce289c
9e859e6
f1e70d8
26839d7
cbdca58
8ba995c
411fd7a
ae6ae6e
97cebf2
a794bd0
3e04b9f
068d16b
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 |
|---|---|---|
|
|
@@ -273,7 +273,7 @@ describe('hash', () => { | |
|
|
||
| map.setCenter([2.0, 1.0]); | ||
|
|
||
| expect(window.location.hash).toBe('#baz&map=7/1/2/135/60&foo=bar'); | ||
| expect(window.location.hash).toBe('#baz=&map=7/1/2/135/60&foo=bar'); | ||
|
CommanderStorm marked this conversation as resolved.
Outdated
|
||
| }); | ||
|
|
||
| describe('_removeHash without a name', () => { | ||
|
|
@@ -335,7 +335,7 @@ describe('hash', () => { | |
|
|
||
| hash._removeHash(); | ||
|
|
||
| expect(window.location.hash).toBe('#baz&foo=bar'); | ||
| expect(window.location.hash).toBe('#baz=&foo=bar'); | ||
|
CommanderStorm marked this conversation as resolved.
Outdated
|
||
| }); | ||
| }); | ||
|
|
||
|
|
@@ -383,10 +383,10 @@ describe('hash', () => { | |
| expect(hash._isValidHash(hash._getCurrentHash())).toBeTruthy(); | ||
| }); | ||
|
|
||
| test('invalidate hash with slashes encoded as %2F', () => { | ||
| test('validate hash with slashes encoded as %2F', () => { | ||
| window.location.hash = '#10%2F3.00%2F-1.00'; | ||
|
|
||
| expect(hash._isValidHash(hash._getCurrentHash())).toBeFalsy(); | ||
| expect(hash._isValidHash(hash._getCurrentHash())).toBeTruthy(); | ||
|
Contributor
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. I’d characterize this as a routine bug fix rather than a backwards-incompatible change. I don’t think anyone would be expecting the
Collaborator
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. A case where this is needed for example is if you have a link to a file or location as part of the query parameters, then you can't use "/" but you have to use the encoded way. I'm not sure this answers any question, but I wanted to give a use case. |
||
| }); | ||
|
|
||
| test('invalidate hash with string values', () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,40 +65,26 @@ | |
| if (pitch) hash += (`/${Math.round(pitch)}`); | ||
|
|
||
| if (this._hashName) { | ||
| const hashName = this._hashName; | ||
| let found = false; | ||
| const parts = window.location.hash.slice(1).split('&').map(part => { | ||
| const key = part.split('=')[0]; | ||
| if (key === hashName) { | ||
| found = true; | ||
| return `${key}=${hash}`; | ||
| } | ||
| return part; | ||
| }).filter(a => a); | ||
| if (!found) { | ||
| parts.push(`${hashName}=${hash}`); | ||
| } | ||
| return `#${parts.join('&')}`; | ||
| const params = this._getHashParams(); | ||
| params.set(this._hashName, hash); | ||
| return `#${decodeURIComponent(params.toString())}`; | ||
|
CommanderStorm marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| return `#${hash}`; | ||
| } | ||
|
|
||
| _getHashParams = () => { | ||
| return new URLSearchParams(window.location.hash.replace('#', '')); | ||
|
Contributor
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. If |
||
| }; | ||
|
CommanderStorm marked this conversation as resolved.
|
||
|
|
||
| _getCurrentHash = () => { | ||
| // Get the current hash from location, stripped from its number sign | ||
| const hash = window.location.hash.replace('#', ''); | ||
| const params = this._getHashParams(); | ||
| if (this._hashName) { | ||
| // Split the parameter-styled hash into parts and find the value we need | ||
| let keyval; | ||
| hash.split('&').map( | ||
| part => part.split('=') | ||
| ).forEach(part => { | ||
| if (part[0] === this._hashName) { | ||
| keyval = part; | ||
| } | ||
| }); | ||
| return (keyval ? keyval[1] || '' : '').split('/'); | ||
| return (params.get(this._hashName) || '').split('/'); | ||
| } | ||
|
CommanderStorm marked this conversation as resolved.
|
||
| // For unnamed hashes, get the first key | ||
| const keys = Array.from(params.keys()); | ||
| const hash = keys.length > 0 ? keys[0] : ''; | ||
|
CommanderStorm marked this conversation as resolved.
Outdated
|
||
| return hash.split('/'); | ||
| }; | ||
|
|
||
|
|
@@ -121,30 +107,25 @@ | |
| }; | ||
|
|
||
| _updateHashUnthrottled = () => { | ||
| // Replace if already present, else append the updated hash string | ||
| const location = window.location.href.replace(/(#.*)?$/, this.getHashString()); | ||
| window.history.replaceState(window.history.state, null, location); | ||
| }; | ||
|
|
||
| _removeHash = () => { | ||
| const currentHash = this._getCurrentHash(); | ||
| if (currentHash.length === 0) { | ||
| return; | ||
| } | ||
| const baseHash = currentHash.join('/'); | ||
| let targetHash = baseHash; | ||
| if (targetHash.split('&').length > 0) { | ||
| targetHash = targetHash.split('&')[0]; // #3/1/2&foo=bar -> #3/1/2 | ||
| } | ||
| const params = this._getHashParams(); | ||
|
|
||
| if (this._hashName) { | ||
| targetHash = `${this._hashName}=${baseHash}`; | ||
| } | ||
| let replaceString = window.location.hash.replace(targetHash, ''); | ||
| if (replaceString.startsWith('#&')) { | ||
| replaceString = replaceString.slice(0, 1) + replaceString.slice(2); | ||
| } else if (replaceString === '#') { | ||
| replaceString = ''; | ||
| params.delete(this._hashName); | ||
| } else { | ||
| // For unnamed hash (#zoom/lat/lng&other=params), remove first entry | ||
| const keys = Array.from(params.keys()); | ||
| if (keys.length > 0) { | ||
| params.delete(keys[0]); | ||
| } | ||
| } | ||
|
|
||
| const newHash = decodeURIComponent(params.toString()); | ||
|
CommanderStorm marked this conversation as resolved.
Outdated
|
||
| const replaceString = newHash ? `#${newHash}` : '' | ||
| let location = window.location.href.replace(/(#.+)?$/, replaceString); | ||
| location = location.replace('&&', '&'); | ||
| window.history.replaceState(window.history.state, null, location); | ||
|
|
@@ -155,8 +136,8 @@ | |
| */ | ||
| _updateHash: () => ReturnType<typeof setTimeout> = throttle(this._updateHashUnthrottled, 30 * 1000 / 100); | ||
|
|
||
| _isValidHash(hash: number[]) { | ||
| if (hash.length < 3 || hash.some(isNaN)) { | ||
| _isValidHash(hash: string[]) { | ||
| if (hash.length < 3 || hash.some(h => isNaN(+h))) { | ||
| return false; | ||
| } | ||
|
|
||
|
|
||
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.
Why was this changed? Is it possible that this might be a "breaking change"?
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.
I have no clue.
This change is the result of simplifying
_buildHashStringtodecodeURIComponent(params.toString())https://github.com/maplibre/maplibre-gl-js/pull/7073/changes/578887c634715eb49f2bcc8bad205041381c0017..615b59c48a350538bdfeb4f76ccbe3a1900bd617
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.
Apparently
foo=is the canonical form offoo.URLSearchParamsdoesn’t distinguish. I’m pretty sure this is consistent with how most software interprets query strings, but here we’re reusing query strings as fragments. Technically we get to decide the correct behavior of flag parameters. As long as we useURLSearchParamsfor both parsing and reconstructing the fragment, it shouldn’t cause any problems but could trip up tests like this one.