refactor!: `UrlSearchParams` based `Hash` by CommanderStorm · Pull Request #7073 · maplibre/maplibre-gl-js

CommanderStorm · 2026-02-07T14:23:58Z

Launch Checklist

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Write tests for all new functionality.

This PR refactors Hash to be based on UrlSearchParams.
Tests were pulled to another PR.

Resolves #7006

codecov · 2026-02-07T14:43:29Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.34%. Comparing base (5a376f6) to head (068d16b).
⚠️ Report is 47 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7073      +/-   ##
==========================================
- Coverage   92.62%   92.34%   -0.28%     
==========================================
  Files         289      289              
  Lines       24013    23996      -17     
  Branches     5086     5083       -3     
==========================================
- Hits        22241    22160      -81     
- Misses       1772     1836      +64

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

This reverts commit 80f0973.

1ec5 · 2026-02-07T23:18:23Z

+            // Manually build the string to avoid URL encoding the hash value
+            return `#${this._buildHashString(params)}`;


If the issue is only that / gets escaped despite being legal in the URL fragment, would replacing %2F have the desired effect?

return params.toString().replaceAll('%2F', '/');

Otherwise, unescaping the string should be equivalent, avoiding the need to build a string manually:

return decodeURIComponent(params.toString());

HarelM · 2026-02-08T06:57:36Z

        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');


Why was this changed? Is it possible that this might be a "breaking change"?

I have no clue.
This change is the result of simplifying _buildHashString to decodeURIComponent(params.toString())
https://github.com/maplibre/maplibre-gl-js/pull/7073/changes/578887c634715eb49f2bcc8bad205041381c0017..615b59c48a350538bdfeb4f76ccbe3a1900bd617

Apparently foo= is the canonical form of foo. URLSearchParams doesn’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 use URLSearchParams for both parsing and reconstructing the fragment, it shouldn’t cause any problems but could trip up tests like this one.

HarelM · 2026-02-08T06:59:32Z

+            .addTo(map);
+
+        // Set up hash with URL in another parameter
+        window.location.hash = '#map=10/3/-1&returnUrl=https://example.com&filter=a&b=';


What happens if you have https://example.com/&filter=a&b= instead (note the addition of "/" after .com)?

HarelM · 2026-02-08T07:00:39Z

+        // Update and verify encoding is preserved where needed
+        map.setZoom(8);
+
+        expect(window.location.hash).toBe('#map=8/3/-1&foo=bar');


This doesn't look like it preserves the "%2F", am I missing anything?

No, URL encoding is getting removed

I think it makes sense to continue to unescape %2F, since it gets annoying in the usual map= parameter. Slashes in the URL fragment don’t need to be escaped anyhow. However, other encoding might be useful. The class is written generically enough that a parameter could end up with freeform text containing a & that needs to be escaped. My other suggestion in #7073 (comment) would’ve solved this problem, prettifying %2F as / but leaving all the other escaping in place.

The other character to watch out for is the space, which URLSearchParams encodes as + rather than %20.

CommanderStorm · 2026-02-08T12:17:04Z

Lets split adding tests and changing behaviour, this way the actual changes are more clear

Co-authored-by: Minh Nguyễn <mxn@1ec5.org>

HarelM · 2026-02-10T21:44:44Z

I'm a bit lost with all these commits and review comments, please ping me when a new review is required.

CommanderStorm · 2026-02-11T00:52:12Z

@HarelM the PR is done, sorry for the messy history, not my best work.

So basically, this PR boils down to "how should this be normalized and should this be normalized?"

if yes, then the previous version
if no, then the current version
if shmaybe (neighter one), then the previous version. See tests what I mean

We can aso put this into v6

HarelM · 2026-02-13T08:21:59Z

I think using urlsearchpatams is important, but this is a subtle breaking change.
Let's add that to the list of v6 breaking changes.
I want to push v6 forward, we have enough reasons to release a breaking change version and we haven't released one in over a year, so it's time.

HarelM · 2026-02-13T08:30:41Z

I've added it to the list.

1ec5 · 2026-02-13T11:06:05Z

We can easily avoid the breaking change if desired with a one-liner. That was the question in #7073 (comment).

HarelM · 2026-02-13T11:09:15Z

I might have miss read it then, avoiding a breaking change would be better, we can introduce the breaking change later if it's only a single line change.

CommanderStorm · 2026-02-13T13:17:46Z

I don't think this can be done without a subtle behaviour change if we want to use UrlSearchParams, that is what I meant by previous version.

1ec5 · 2026-02-15T02:43:43Z

            window.location.hash = '#10%2F3.00%2F-1.00';

-            expect(hash._isValidHash(hash._getCurrentHash())).toBeFalsy();
+            expect(hash._isValidHash(hash._getCurrentHash())).toBeTruthy();


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.

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.

1ec5 · 2026-02-15T03:03:39Z

    }

+    _getHashParams = () => {
+        return new URLSearchParams(window.location.hash.replace('#', ''));


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

1ec5 · 2026-02-15T03:21:28Z


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


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…

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 😀

HarelM · 2026-02-18T10:12:50Z

I'm a bit lost, where are we with this PR? Waiting for version 6?

CommanderStorm · 2026-02-18T16:50:26Z

Yes, I think that is the best bet.

CommanderStorm requested a review from Copilot February 7, 2026 14:40

Copilot started reviewing on behalf of CommanderStorm February 7, 2026 14:40 View session

This comment was marked as resolved.

Sign in to view

Add tests for Hash

18ab1a8

CommanderStorm force-pushed the URLSearchParams-based-hash branch from 17f4a79 to 18ab1a8 Compare February 7, 2026 14:46

CommanderStorm added 2 commits February 7, 2026 15:47

Migrate to using UrlSearchParams

bd37fe2

Add test that only passes after

53977e2

CommanderStorm marked this pull request as ready for review February 7, 2026 14:47

CommanderStorm and others added 4 commits February 7, 2026 15:47

Merge branch 'main' into URLSearchParams-based-hash

9a4aa80

Update src/ui/hash.test.ts

661f343

Apply suggestion from @Copilot

80f0973

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Revert "Apply suggestion from @Copilot"

578887c

This reverts commit 80f0973.

1ec5 reviewed Feb 7, 2026

View reviewed changes

CommanderStorm added 3 commits February 8, 2026 00:54

remove _buildHashString

2cc15ba

fix typo

be00422

simplify a bit more

a3fa7b6

HarelM reviewed Feb 8, 2026

View reviewed changes

Comment thread src/ui/hash.ts Outdated

HarelM reviewed Feb 8, 2026

View reviewed changes

Comment thread src/ui/hash.test.ts Outdated

HarelM reviewed Feb 8, 2026

View reviewed changes

Comment thread src/ui/hash.test.ts Outdated

HarelM reviewed Feb 8, 2026

View reviewed changes

Comment thread src/ui/hash.test.ts Outdated

CommanderStorm commented Feb 8, 2026

View reviewed changes

Comment thread src/ui/hash.ts Outdated

CommanderStorm added 2 commits February 8, 2026 11:19

Apply suggestion from @CommanderStorm

615b59c

Merge branch 'main' into URLSearchParams-based-hash

919d058

CommanderStorm mentioned this pull request Feb 8, 2026

test: More thoroughly test Hash #7078

Merged

1 task

CommanderStorm marked this pull request as draft February 8, 2026 12:16

Apply suggestion from @CommanderStorm

8ba995c

1ec5 approved these changes Feb 10, 2026

View reviewed changes

Comment thread src/ui/hash.ts Outdated

Comment thread src/ui/hash.ts Outdated

Update src/ui/hash.ts

411fd7a

Co-authored-by: Minh Nguyễn <mxn@1ec5.org>

CommanderStorm commented Feb 10, 2026

View reviewed changes

Comment thread src/ui/hash.test.ts Outdated

CommanderStorm commented Feb 10, 2026

View reviewed changes

Comment thread src/ui/hash.test.ts Outdated

Apply suggestions from code review

ae6ae6e

Co-authored-by: Minh Nguyễn <mxn@1ec5.org>

CommanderStorm commented Feb 10, 2026

View reviewed changes

Comment thread src/ui/hash.ts Outdated

Apply suggestion from @CommanderStorm

97cebf2

CommanderStorm added 3 commits February 11, 2026 01:46

fix lints

a794bd0

update changed tests names

3e04b9f

Merge branch 'main' into URLSearchParams-based-hash

068d16b

CommanderStorm changed the title ~~refactor: UrlSearchParams based Hash~~ refactor!: UrlSearchParams based Hash Feb 11, 2026

pierreeurope mentioned this pull request Feb 12, 2026

refactor: simplify Hash class with structured parsing helpers #7113

Closed

HarelM mentioned this pull request Feb 13, 2026

v6 breaking changes features #6427

Open

CommanderStorm requested a review from 1ec5 February 13, 2026 13:18

1ec5 reviewed Feb 15, 2026

View reviewed changes

HarelM added the need more info Further information is requested label Feb 18, 2026

CommanderStorm removed the need more info Further information is requested label Feb 18, 2026

CommanderStorm added this to the 6.0 milestone Feb 18, 2026

CommanderStorm added benchmark Related to testing performance and removed benchmark Related to testing performance labels Feb 26, 2026

		// Manually build the string to avoid URL encoding the hash value
		return `#${this._buildHashString(params)}`;

Uh oh!

Conversation

CommanderStorm commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Launch Checklist

Uh oh!

codecov bot commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CommanderStorm commented Feb 8, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HarelM commented Feb 10, 2026

Uh oh!

CommanderStorm commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HarelM commented Feb 13, 2026

Uh oh!

HarelM commented Feb 13, 2026

Uh oh!

1ec5 commented Feb 13, 2026

Uh oh!

HarelM commented Feb 13, 2026

Uh oh!

CommanderStorm commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HarelM commented Feb 18, 2026

Uh oh!

CommanderStorm commented Feb 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

CommanderStorm commented Feb 7, 2026 •

edited

Loading

codecov bot commented Feb 7, 2026 •

edited

Loading

CommanderStorm commented Feb 11, 2026 •

edited

Loading