Combine Router and Renderer
#4161
Conversation
|
There was a problem hiding this comment.
This still feels like throwing the baby out with the bathwater to me. I totally agree the current classes are terribly structured, but I think there are better solutions than putting everything into a single file. At the end of the day, I might just have to agree to disagree, but I'd love if we could get other opinions on this approach vs #4101
| let scroll_positions = {}; | ||
| try { | ||
| scroll_positions = JSON.parse(sessionStorage[SCROLL_KEY]); | ||
| } catch { | ||
| // do nothing | ||
| } |
There was a problem hiding this comment.
I think this could be a const as it doesn't appear to be updated after initialization. E.g. you could do something like:
const scroll_positions = (() => {
try {
return JSON.parse(sessionStorage[SCROLL_KEY]);
} catch {}
return {};
})();
There was a problem hiding this comment.
I've always felt iffy about IIFEs
There was a problem hiding this comment.
Another option to make it a const might be:
const scroll_positions = {};
try {
Object.assign(scroll_positions, JSON.parse(sessionStorage[SCROLL_KEY]));
} catch {
// do nothing
}
| } | ||
|
|
||
| /** @param {URL} url */ | ||
| function _parse(url) { |
There was a problem hiding this comment.
I think _parse doesn't quite represent what this method does or is a bit vaguely named. How about _get_navigation_candidates?
There was a problem hiding this comment.
changed to get_navigation_intent
| /** @type {Record<string, any>} */ | ||
| let stuff = {}; | ||
|
|
||
| /** @type {import('./types').NavigationResult | undefined} */ |
There was a problem hiding this comment.
I think we should rename NavigationResult to LoadResult
There was a problem hiding this comment.
Agree that the terminology could be tightened up a bit, though I think there's value in thinking of a 'load' as something that relates to a single node (i.e. layout/page component) in the branch, since that's where you might find a load function
Co-authored-by: Maurício Kishi <mrkishi@users.noreply.github.com>
|
|
||
| /** @type {import('./types').NavigationState} */ | ||
| let current = { | ||
| // @ts-ignore - we need the initial value to be null |
There was a problem hiding this comment.
is this true? could we not just do new URL(window.location.href)?
There was a problem hiding this comment.
Conceptually, upon hydration we're navigating from null to location.href, and this is reflected in places like
kit/packages/kit/src/runtime/client/client.js
Line 396 in 7373dd5
kit/packages/kit/src/runtime/client/client.js
Lines 564 to 568 in 7373dd5
| }; | ||
|
|
||
| /** @type {{id: string | null, promise: Promise<import('./types').NavigationResult | undefined> | null}} */ | ||
| const loading = { |
There was a problem hiding this comment.
a better name for this might be load_cache
a related note - I just saw someone on Discord raise the issue that if they move their mouse between two links they will be prefetched over and over again. possibly we could make this more than a single-membered cache in the future
There was a problem hiding this comment.
renamed. re multiple prefetches, the current behaviour is by design, but if they want to open an issue then we can discuss further
| if (!ready) return; | ||
| session_id += 1; | ||
|
|
||
| const intent = get_navigation_intent(new URL(location.href)); |
There was a problem hiding this comment.
should we just call invalidate here?
There was a problem hiding this comment.
It doesn't have quite the right semantics — if session changes we want to re-run load functions that depend on session, not those that fetch(location.href)
| const current_token = (token = {}); | ||
| let navigation_result = await get_navigation_result(intent, no_cache); | ||
|
|
||
| if (!navigation_result && intent.url.pathname === location.pathname) { |
There was a problem hiding this comment.
something that might be worth a comment - I'm not understanding the second half of this check. if you want to navigate to the page we're already on, isn't that essentially just a no-op? why do we want to show a 404 page in that case?
| } | ||
|
|
||
| // abort if user navigated during update | ||
| if (token !== current_token) return; |
There was a problem hiding this comment.
I noticed when working on #4101 that this token stuff looks broken in master. I didn't attempt to fix it and it looks like it's not addressed here either. Assuming I haven't missed something, current_token is only ever set to {} and so this is a no-op check
There was a problem hiding this comment.
update() can be called by a different navigation/redirect between the time it started and the time this check is made, meaning token will point to a different {} than this invocation's current_token, and in that case we abort (because this call to update() was superseded).
There was a problem hiding this comment.
it's working as intended as far as I can see. consider this simplified version:
let token;
async function log(message) {
const current_token = (token = {});
await Promise.resolve();
if (token !== current_token) return;
console.log(message);
}
// only b is logged; a is aborted
log('a');
log('b');There was a problem hiding this comment.
Oh. I see. That's a bit of a subtle thing that relies on === checking that they're the same instance. I was just looking at it and seeing that they were both always {} and doing more of a checking that the values were equivalent in my head. Maybe we could either add a comment or rename to current_token_instance and token_instance?
There was a problem hiding this comment.
I think it's clear enough, given the comment immediately above the check
This has been on my wishlist for a while — taking the
RouterandRendererclasses, which are somewhat tightly coupled and have a confusing relationship to each other, and converting them to a singlecliententity. It does mean a big assclient.jsfile (though we could probably extract out some utilities), but overall I think it's much easier to understand the flow of things when it's all colocated.I opted for a revealing module pattern in this PR rather than a new
classfor a couple of reasons:thisnonsense —$app/navigationcan simply re-export the public methods without needing to wrap themThere's still some tidying up to do, but on the whole this feels like a good direction to me — less source code, less built code, less indirection.
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
pnpx changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0