fix: change page.url when calling pushState(...) with a new url #13221
Conversation
|
| @@ -1958,17 +1958,22 @@ export function pushState(url, state) { | |||
| } | |||
|
|
|||
| update_scroll_positions(current_history_index); | |||
| url = resolve_url(url); | |||
There was a problem hiding this comment.
to make sure that a new instance of URL is created as resolve_url will simply return the same instance if url was already a URL vs a string. And if the instance is the same replacing url on the page will not trigger a change.
| url = resolve_url(url); | |
| url = new URL(resolve_url(url)); |
| has_navigated = true; | ||
|
|
||
| if (change) { | ||
| page.url = url; |
There was a problem hiding this comment.
Not 100% sure but maybe update_url(url) would be the call here to make sure that stores are also updated.
| page.url = url; | |
| update_url(url); |
There was a problem hiding this comment.
looks like the same changes as for pushState should be applied to export function replaceState(url, state). They seem to be very much the same and probably should share a common function with an arg like method with values either being pushState or replaceState. The error messages would be the same with method replacements and then there is only other thing when calling history and should be taken care of with history[method](opts, '', url).
But I don't know if the team still wants to keep them separate.
| @@ -1958,17 +1958,22 @@ export function pushState(url, state) { | |||
| } | |||
|
|
|||
| update_scroll_positions(current_history_index); | |||
| url = resolve_url(url); | |||
| const change = page.url.href !== url.href ? 1 : 0; | |||
There was a problem hiding this comment.
it's a boolean so not needed this here.
| const change = page.url.href !== url.href ? 1 : 0; |
| has_navigated = true; | ||
|
|
||
| if (change) { |
There was a problem hiding this comment.
since it's a boolean
| if (change) { | |
| if (page.url.href !== url.href) { |
|
|
||
| const opts = { | ||
| [HISTORY_INDEX]: (current_history_index += 1), | ||
| [NAVIGATION_INDEX]: current_navigation_index, | ||
| [PAGE_URL_KEY]: page.url.href, | ||
| [NAVIGATION_INDEX]: (current_navigation_index += change), |
There was a problem hiding this comment.
I don't think this should be updated since no navigation actually takes place, only the url changes.
| [NAVIGATION_INDEX]: (current_navigation_index += change), | |
| [NAVIGATION_INDEX]: current_navigation_index, |
| type="button" | ||
| class="after" | ||
| onclick={() => { | ||
| pushState('/state/url/after', { s: 'test' }); |
There was a problem hiding this comment.
also need a test to pushState with new URL(...) to check if it's reactive with a URL instance.
|
@Leonidaz really good points, thank you! Will try to work my way through them with tests first approach tomorrow. Even if the team will reject the PR, my goal is first of all to learn a bit more here with the hopes to contribute something meaningful to svelte one day :) |
|
closing in favour of #13205 |
Recently @Rich-Harris fixed the first part of the bug, that I reported in #13196. However the second bug related to
pushStateis still a problem.Basically when doing
pushState('/new-url', { foo: 'bar' })you end up with real browser url andpage.urlbeing out of sync. The weird part is that some of the tests are explicitly ensuring current behavior, which makes me unsure if that's an accident or I am missing something.Some tests are failing because their existence made me unsure about the change and I am opening this PR to validate it.
That's my first contribution. I've tried to match the code and logic written around, but am not sure how well I've done that. Happy to incorporate any feedback.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits