Ignore non-native LocationChanges in native navigation effect#441
Closed
Brendonovich wants to merge 4 commits intosolidjs:mainfrom
Closed
Ignore non-native LocationChanges in native navigation effect#441Brendonovich wants to merge 4 commits intosolidjs:mainfrom
Brendonovich wants to merge 4 commits intosolidjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 83d7125 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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? |
Contributor
Author
|
#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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
createRenderEffectin 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:submissions[1]([])onto the next microtask by wrapping it instartfor a transition.onMountruns (post-render phase), invoking the action and creating an entry insubmissions.submissions[1]([])runs, wiping out the previously created entry insubmissions.I've fixed this by adding an internal
intentproperty toLocationChange, which if present instructs the render effect to actually run, so it will not run on page load or fromnavigate(). The property has@internalso it doesn't show up in the generated.d.ts.Fixes #433
I also ran the
prettyscript 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.