Skip to content

Ignore non-native LocationChanges in native navigation effect#441

Closed
Brendonovich wants to merge 4 commits intosolidjs:mainfrom
Brendonovich:fix-433
Closed

Ignore non-native LocationChanges in native navigation effect#441
Brendonovich wants to merge 4 commits intosolidjs:mainfrom
Brendonovich:fix-433

Conversation

@Brendonovich
Copy link
Contributor

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
Copy link

changeset-bot bot commented Jun 8, 2024

🦋 Changeset detected

Latest commit: 83d7125

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
Copy link
Member

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
Copy link
Contributor Author

Brendonovich commented Jun 12, 2024

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Actions called in onMount (or first run effect) are destroyed by start process, and do not receive values nor errors

2 participants