fix: state updates detection on history back/forward

[oedotme]

This PR adds an event listener to popstate change event to update the router state with the history back/forward events between two routes with the same paths but with different state objects.

Currently the changes are only detected by refreshing the page.

Closes #275 and #396

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 82f80358f73e585417b8c9c44461ebda7c6dbd9a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | --------------- | ----- | | @solidjs/router | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ryansolid]

I see what you are trying to do but could this already be handled by the current popstate event? I'm gathering this is related to that we don't have access to the state representation where we wire the events.

I'm just looking at: https://github.com/solidjs/solid-router/blob/3cd2fef67fe0b4d8bb3178923be32d541d56ec79/src/routers/Router.ts#L29-L38

And more specifically: https://github.com/solidjs/solid-router/blob/3cd2fef67fe0b4d8bb3178923be32d541d56ec79/src/lifecycle.ts#L64-L77

And thinking there should be some way to set the source state properly and ensure that the notifications are working properly. Like the bug seems most likely with our way of telling things have changed rather than sideloading in a second event.

[oedotme]

@ryansolid Thanks for your response, and yeah that makes sense.

From what I found so far, the source signal doesn't trigger updates when the state changes while the path value is the same, because of the following equals condition:

https://github.com/solidjs/solid-router/blob/3cd2fef67fe0b4d8bb3178923be32d541d56ec79/src/routers/createRouter.ts#L35

To allow the state change detection, we can set equals to:

• false
• or (a, b) => a.value === b.value && JSON.stringify(a.value) === JSON.stringify(b.value)
• or even JSON.stringify(a) === JSON.stringify(b)

I went with false to avoid JSON.stringify potential performance hits, but please let me know what you think:

-       createSignal(wrap(config.get()), { equals: (a, b) => a.value === b.value }),
+       createSignal(wrap(config.get()), { equals: false }),

The other related logic that skips updates if the path hasn't changed:

https://github.com/solidjs/solid-router/blob/3cd2fef67fe0b4d8bb3178923be32d541d56ec79/src/routing.ts#L333-L343

I was unsure about resetting error-boundaries and submission here as well, so I just added an else statement in order to handle the state-only changes:

+      } else {
+        start(() => {
+          intent = "native";
+          setState(state);
+        }).then(() => {
+          intent = undefined;
+        });
      }

Those two changes solved the state-only detection issue. Please let me know what you think, thanks!

[ryansolid]

I see.. hmm.. yeah that equality check I guess needs to fail here but I do wonder if this can cause other issues. Shortcuts when trying to navigate to the same route. Probably nothing to sever.

We should reset errorboundaries and submissions in this case, because it is a navigation.

[oedotme]

@ryansolid I re-added the equals check again just in case, to avoid any potential unexpected behaviors.

I also added an object equality comparison utility based on fast-deep-equal to compare the state, which addresses the performance concern I initially had with JSON.stringify.

Please let me know what you think. Thanks!

[ryansolid]

I'm going to play around with this and see if the equal check is important. I'm as concerned with adding more code as I am with JSON.stringify performance. So if we can avoid doing much here. My gut here is if the state reference is different that will be sufficient. It might be aggressive but it is probably fine. Like what would be the point of state if someone was just mutating the same reference. You wouldn't be able to go back and forth, as you already changed it.

Could it not just be?

equals: (a, b) => a.value === b.value && a.state === b.state }

[oedotme]

@ryansolid Checking the state reference makes total sense in this case.

Could it not just be?

equals: (a, b) => a.value === b.value && a.state === b.state }

Yes, I've update the PR accordingly. Thanks again.

[ryansolid]

Looks great thanks