Skip to content

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.

Sign up for GitHub

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

Open
CommanderStorm wants to merge 27 commits into
base: main
Choose a base branch
from
Open

refactor!: UrlSearchParams based Hash #7073

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
18ab1a8
Add tests for Hash
CommanderStorm Feb 7, 2026
bd37fe2
Migrate to using UrlSearchParams
CommanderStorm Feb 7, 2026
53977e2
Add test that only passes after
CommanderStorm Feb 7, 2026
9a4aa80
Merge branch 'main' into URLSearchParams-based-hash
CommanderStorm Feb 7, 2026
661f343
Update src/ui/hash.test.ts
CommanderStorm Feb 7, 2026
80f0973
Apply suggestion from @Copilot
CommanderStorm Feb 7, 2026
578887c
Revert "Apply suggestion from @Copilot"
CommanderStorm Feb 7, 2026
2cc15ba
remove _buildHashString
CommanderStorm Feb 7, 2026
be00422
fix typo
CommanderStorm Feb 8, 2026
a3fa7b6
simplify a bit more
CommanderStorm Feb 8, 2026
615b59c
Apply suggestion from @CommanderStorm
CommanderStorm Feb 8, 2026
919d058
Merge branch 'main' into URLSearchParams-based-hash
CommanderStorm Feb 8, 2026
38a0c92
Split some tests
CommanderStorm Feb 8, 2026
f59809a
Merge branch 'main' into URLSearchParams-based-hash
CommanderStorm Feb 9, 2026
85dcd78
fix merge issue
CommanderStorm Feb 9, 2026
9ce289c
split tests
CommanderStorm Feb 9, 2026
9e859e6
refactor _getCurrentHash to use the params
CommanderStorm Feb 9, 2026
f1e70d8
update tests
CommanderStorm Feb 9, 2026
26839d7
Merge branch 'main' into URLSearchParams-based-hash
CommanderStorm Feb 10, 2026
cbdca58
Merge branch 'main' into URLSearchParams-based-hash
CommanderStorm Feb 10, 2026
8ba995c
Apply suggestion from @CommanderStorm
CommanderStorm Feb 10, 2026
411fd7a
Update src/ui/hash.ts
CommanderStorm Feb 10, 2026
ae6ae6e
Apply suggestions from code review
CommanderStorm Feb 10, 2026
97cebf2
Apply suggestion from @CommanderStorm
CommanderStorm Feb 10, 2026
a794bd0
fix lints
CommanderStorm Feb 11, 2026
3e04b9f
update changed tests names
CommanderStorm Feb 11, 2026
068d16b
Merge branch 'main' into URLSearchParams-based-hash
CommanderStorm Feb 11, 2026
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: 2 additions & 2 deletions src/ui/hash.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@

map.setCenter([2.0, 1.0]);

expect(window.location.hash).toBe('#baz&map=7/1/2/135/60&foo=bar');

Check failure on line 276 in src/ui/hash.test.ts

View workflow job for this annotation

GitHub Actions / Unit tests

[unit] src/ui/hash.test.ts > hash > _updateHash named 

AssertionError: expected '#bazmap=7/1/2/135/60&foo=bar' to be '#baz&map=7/1/2/135/60&foo=bar' // Object.is equality

Expected: "#baz&map=7/1/2/135/60&foo=bar"
Received: "#bazmap=7/1/2/135/60&foo=bar"

 ❯ src/ui/hash.test.ts:276:38
});

describe('_removeHash without a name', () => {
Expand Down Expand Up @@ -335,7 +335,7 @@

hash._removeHash();

expect(window.location.hash).toBe('#baz&foo=bar');

Check failure on line 338 in src/ui/hash.test.ts

View workflow job for this annotation

GitHub Actions / Unit tests

[unit] src/ui/hash.test.ts > hash > _removeHash with a name > removes hash when hash contains other parameters at start and end 

AssertionError: expected '#baz=&foo=bar' to be '#baz&foo=bar' // Object.is equality

Expected: "#baz&foo=bar"
Received: "#baz=&foo=bar"

 ❯ src/ui/hash.test.ts:338:42
});
});

Expand Down Expand Up @@ -383,10 +383,10 @@
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();
Copy link
Copy Markdown
Contributor

@1ec5 1ec5 Feb 15, 2026

Choose a reason for hiding this comment

The 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 %2F to persist, and we continue to recognize it anyways.

Copy link
Copy Markdown
Collaborator

@HarelM HarelM Feb 15, 2026

Choose a reason for hiding this comment

The 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.
If this is kept the same and doesn't break, that's great.

});

test('invalidate hash with string values', () => {
Expand Down Expand Up @@ -533,7 +533,7 @@
map.setZoom(5);
map.setCenter([1.0, 2.0]);

expect(window.location.hash).toBe('#map=5/2/1&returnUrl=https://example.com&filter=a&b=');

Check failure on line 536 in src/ui/hash.test.ts

View workflow job for this annotation

GitHub Actions / Unit tests

[unit] src/ui/hash.test.ts > hash > hash with URL in other parameter does not change 

AssertionError: expected '#map=5/2/1&returnUrl=https://example.…' to be '#map=5/2/1&returnUrl=https://example.…' // Object.is equality

Expected: "#map=5/2/1&returnUrl=https://example.com&filter=a&b="
Received: "#map=5/2/1&returnUrl=https://example.com&filter=a&b"

 ❯ src/ui/hash.test.ts:536:38

window.location.hash = '#search=foo&map=7/4/2&redirect=/path?query=value';
hash._onHashChange();
Expand All @@ -551,7 +551,7 @@
map.setZoom(5);
map.setCenter([1.0, 2.0]);

expect(window.location.hash).toBe('#map=5/2/1&returnUrl=https://example.com/abcd/ef&filter=a&b=');

Check failure on line 554 in src/ui/hash.test.ts

View workflow job for this annotation

GitHub Actions / Unit tests

[unit] src/ui/hash.test.ts > hash > hash with URL+path in other parameter does not change 

AssertionError: expected '#map=5/2/1&returnUrl=https://example.…' to be '#map=5/2/1&returnUrl=https://example.…' // Object.is equality

Expected: "#map=5/2/1&returnUrl=https://example.com/abcd/ef&filter=a&b="
Received: "#map=5/2/1&returnUrl=https://example.com/abcd/ef&filter=a&b"

 ❯ src/ui/hash.test.ts:554:38

window.location.hash = '#search=foo&map=7/4/2&redirect=/path?query=value';
hash._onHashChange();
Expand Down Expand Up @@ -610,7 +610,7 @@
expect(map.getZoom()).toBe(10);

map.setZoom(5);
expect(window.location.hash).toBe('#map=5/3/-1&empty=');

Check failure on line 613 in src/ui/hash.test.ts

View workflow job for this annotation

GitHub Actions / Unit tests

[unit] src/ui/hash.test.ts > hash > update to hash with empty parameter values is kept as-is 

AssertionError: expected '#map=5/3/-1&empty' to be '#map=5/3/-1&empty=' // Object.is equality

Expected: "#map=5/3/-1&empty="
Received: "#map=5/3/-1&empty"

 ❯ src/ui/hash.test.ts:613:38
});

describe('geographic boundary values', () => {
Expand Down
70 changes: 25 additions & 45 deletions src/ui/hash.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,40 +65,25 @@
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()).replace(/=&|=$/g, '')}`;
}

return `#${hash}`;
}

_getHashParams = () => {
return new URLSearchParams(window.location.hash.replace('#', ''));
Copy link
Copy Markdown
Contributor

@1ec5 1ec5 Feb 15, 2026

Choose a reason for hiding this comment

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

If hash is set, its first character is guaranteed to be #. The previous code only removed the leading #, whereas this removes any occurrence.

};
Comment thread
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('/');
}
Comment thread
CommanderStorm marked this conversation as resolved.
// For unnamed hashes, get the first key
const hash = [...params.keys()][0] ?? '';
return hash.split('/');
};

Expand All @@ -121,30 +106,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());
Comment thread
CommanderStorm marked this conversation as resolved.
Outdated
const replaceString = newHash ? `#${newHash}` : ''

Check failure on line 127 in src/ui/hash.ts

View workflow job for this annotation

GitHub Actions / Code Hygiene 

Missing semicolon
let location = window.location.href.replace(/(#.+)?$/, replaceString);
location = location.replace('&&', '&');
window.history.replaceState(window.history.state, null, location);
Expand All @@ -155,8 +135,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;
}

Expand Down
Loading