solidjs / solid-router

A universal router for Solid inspired by Ember and React Router
MIT License
1.1k stars 137 forks source link

Ignore non-native LocationChanges in native navigation effect #441

Closed Brendonovich closed 4 weeks ago

Brendonovich commented 1 month ago

The createRenderEffect in charge of resetting submissions when a native navigation occurs currently fires whenever any navigation occurs, including when the router first mounts. Because of this, when a route first loads or is navigated to that invokes an action in onMount, a race condition occurs between the action's submission and the effect's submission reset: 1) The render effect runs (render phase), queueing submissions[1]([]) onto the next microtask by wrapping it in start for a transition. 2) onMount runs (post-render phase), invoking the action and creating an entry in submissions. 3) The next microtask comes and submissions[1]([]) runs, wiping out the previously created entry in submissions.

I've fixed this by adding an internal intent property to LocationChange, which if present instructs the render effect to actually run, so it will not run on page load or from navigate(). The property has @internal so it doesn't show up in the generated .d.ts.

Fixes #433

I also ran the pretty script bc my local formatter messed some formatting up. If needed I can take just the important changes and get rid of all the formatting changes.

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 83d7125d3c81229bd845d5bad0cec2f17f36439b

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 commented 4 weeks ago

I have to admit on its own the formatting would have been fine.. but I'm trying to compare this against #442 so not quite clear the differences are. @Brendonovich do you have any thoughts on #442?

Brendonovich commented 4 weeks ago

442 looks good, it addresses the same things as this PR and some more, I'll close this and suggest some improvements on the other.