-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[fix] route invalidation comparison based on url and route, not on id #8399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 98bd50d 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 |
| const url_changed = current.url | ||
| ? url.pathname + url.search !== current.url.pathname + current.url.search | ||
| : false; | ||
| const url_changed = current.url ? id !== current.url.pathname + current.url.search : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure the id, which serves as an unique identifier for caching from what I see, will always be an URL-type thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the id in that instance (as its comment on hover suggests) is url.pathname + url.search - you can backtrack that variable to get_navigation_intent where it's computed as such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know it is currently pathname + search - I'm just considering:
idname is closer to "route id" than to "URL", which it currently represents in essence - I think that's what caused the initial oversight- we are not sure if down the line that
idwill change to something else more specific, breaking this comparison once again.
So I opted to shed that id dependency - especially because url is still there being returned for its intended purpose, while id's purpose is something else entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that id is probably a misleading name in this instance because of route.id, but that id cannot change during the cause of a route change (only if another route change is triggered as a result of it, at which point a new id will be computed). I think we can keep it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I meant "change" as in "someone changes what is computed inside get_navigation_intent at some point due to a caching requirement change and overlooks this comparison due to destructuring, commits and it's borken again".
Thanks for merging!
|
Thank you! |
Reference issue: #8398
This solves layouts always invalidating and rerunning
loadif they mentionevent.route, even if the route ID did not change, due to a wrong comparison in the code.I have no idea how to write tests, but this should be simple enough.
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. All changesets should bepatchuntil SvelteKit 1.0