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
+33 −53
Open

refactor!: UrlSearchParams based Hash #7073

Show file tree
Hide file tree
Changes from all 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
16 changes: 8 additions & 8 deletions src/ui/hash.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
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 @@ -524,7 +524,7 @@ describe('hash', () => {

});

test('hash with URL in other parameter does not change', () => {
test('hash with URL in other parameter does not change except normalization', () => {
const hash = createHash('map')
.addTo(map);

Expand All @@ -533,7 +533,7 @@ describe('hash', () => {
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=');
expect(window.location.hash).toBe('#map=5/2/1&returnUrl=https://example.com&filter=a&b');

window.location.hash = '#search=foo&map=7/4/2&redirect=/path?query=value';
hash._onHashChange();
Expand All @@ -542,7 +542,7 @@ describe('hash', () => {
expect(map.getCenter().lng).toBe(2);
});

test('hash with URL+path in other parameter does not change', () => {
test('hash with URL+path in other parameter does not change except for normalization', () => {
const hash = createHash('map')
.addTo(map);

Expand All @@ -551,7 +551,7 @@ describe('hash', () => {
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=');
expect(window.location.hash).toBe('#map=5/2/1&returnUrl=https://example.com/abcd/ef&filter=a&b');

window.location.hash = '#search=foo&map=7/4/2&redirect=/path?query=value';
hash._onHashChange();
Expand Down Expand Up @@ -601,7 +601,7 @@ describe('hash', () => {

});

test('update to hash with empty parameter values is kept as-is', () => {
test('update to hash with empty parameter are de-normalized', () => {
const hash = createHash('map')
.addTo(map);

Expand All @@ -610,7 +610,7 @@ describe('hash', () => {
expect(map.getZoom()).toBe(10);

map.setZoom(5);
expect(window.location.hash).toBe('#map=5/3/-1&empty=');
expect(window.location.hash).toBe('#map=5/3/-1&empty');
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.

whatwg/url#427 led me down quite a rabbit hole of discussions…

As I mentioned earlier, I think we could have leeway to make this change. The fact that our hashes generally conform to the query string syntax is just a convention on our part. I also don’t think we necessarily need to hold the hash behavior to the same standard as a formal public API, since Hash isn’t publicly exposed by any APIs yet.

That said, if this is a source of concern nonetheless, anything calling _getHashParams() would need to separately look for flags:

const hash = window.location.hash.slice(1);
const params = new URLSearchParams(hash);
const flags = [...hash.matchAll(/(?<=^|&)(\w+)(?=&|$)/g).map(p => p[1])];
// …
const result = [...params.entries().map(([k, v]) => flags.includes(k) ? k : `${k}=${v}`)].join('&');
return `#${result}`;

Kind of cryptic…

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.

The question is who is relaying on this functionality to work this way, is somewhat a concern I have. The methods in the hash my not be public, setting it as part on map initialization is public and reading the address bar value is public as well.
I'm just saying that waiting a bit longer for version 6 might be safer for this.
It might be regarded as a bug fix though if we want to cut ourselves some slack 😀

});

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 @@ export class Hash {
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, '&').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.

};

_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('/');
}
// For unnamed hashes, get the first key
const hash = [...params.keys()][0] ?? '';
return hash.split('/');
};

Expand All @@ -121,30 +106,25 @@ export class Hash {
};

_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()).replace(/=&/g, '&').replace(/=$/g, '');
const replaceString = newHash ? `#${newHash}` : '';
let location = window.location.href.replace(/(#.+)?$/, replaceString);
location = location.replace('&&', '&');
window.history.replaceState(window.history.state, null, location);
Expand All @@ -155,8 +135,8 @@ export class Hash {
*/
_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