Skip to content
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.

Sign up for GitHub

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

Combine Router and Renderer #4161

Merged
merged 37 commits into from
Mar 2, 2022
Merged

Combine Router and Renderer #4161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
4d334e3
combine Router and Renderer into Client
Rich-Harris Mar 1, 2022
00afff7
shuffle methods around a bit
Rich-Harris Mar 1, 2022
3dd4c99
shuffle things around
Rich-Harris Mar 1, 2022
d0f2bb3
no need to expose routes
Rich-Harris Mar 1, 2022
60c7453
extract utils
Rich-Harris Mar 1, 2022
12d2036
merge master
Rich-Harris Mar 1, 2022
aa41de3
lint
Rich-Harris Mar 1, 2022
e5074d4
rename some methods
Rich-Harris Mar 1, 2022
ca3777a
simplify
Rich-Harris Mar 1, 2022
26757bd
reorder
Rich-Harris Mar 1, 2022
29b0037
reduce indirection
Rich-Harris Mar 1, 2022
d58dc4d
reduce indirection
Rich-Harris Mar 1, 2022
bc1e835
rename NavigationInfo to NavigationIntent
Rich-Harris Mar 1, 2022
fbdcab0
rename _load_error to load_root_error_page
Rich-Harris Mar 1, 2022
8c2ad26
simplify
Rich-Harris Mar 1, 2022
7f9cc40
simplify
Rich-Harris Mar 1, 2022
c787268
rename functions
Rich-Harris Mar 1, 2022
73cd1a3
chain -> redirect_chain
Rich-Harris Mar 1, 2022
72cd8c1
enabled -> router_enabled
Rich-Harris Mar 1, 2022
f850a0b
_navigate -> navigate
Rich-Harris Mar 1, 2022
b431f69
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
97587d7
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
c98a66b
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
6485096
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
1d0434d
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
61f1221
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
e1af2d7
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
0a4d5e1
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 1, 2022
4abafc6
Merge branch 'master' into combined-client
Rich-Harris Mar 2, 2022
a1fe0a1
Update packages/kit/src/runtime/client/client.js
Rich-Harris Mar 2, 2022
8ccdf32
add traditional_navigation helper
Rich-Harris Mar 2, 2022
14b319d
Merge branch 'combined-client' of github.com:sveltejs/kit into combin…
Rich-Harris Mar 2, 2022
7373dd5
appease typescript
Rich-Harris Mar 2, 2022
0be3a5f
loading -> load_cache
Rich-Harris Mar 2, 2022
98ae319
invalid -> invalidated
Rich-Harris Mar 2, 2022
3f8758c
explanatory comment
Rich-Harris Mar 2, 2022
9c00c58
traditional_navigation -> native_navigation
Rich-Harris Mar 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
rename NavigationInfo to NavigationIntent
  • Loading branch information
@Rich-Harris
Rich-Harris committed Mar 1, 2022
commit bc1e835860f8f6ff272ec2c0e99e5ae22237ea1b
94 changes: 46 additions & 48 deletions packages/kit/src/runtime/client/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ export function create_client({ target, session, base, trailing_slash }) {
if (!ready) return;
session_id += 1;

const info = _parse(new URL(location.href));
if (info) _update(info, [], true);
const intent = get_navigation_intent(new URL(location.href));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just call invalidate here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (intent) _update(intent, [], true);
});
ready = true;

Expand Down Expand Up @@ -174,38 +174,38 @@ export function create_client({ target, session, base, trailing_slash }) {

/** @param {URL} url */
async function prefetch(url) {
const info = _parse(url);
const intent = get_navigation_intent(url);

if (!info) {
if (!intent) {
throw new Error('Attempted to prefetch a URL that does not belong to this app');
}

loading.promise = _get_navigation_result(info, false);
loading.id = info.id;
loading.promise = _get_navigation_result(intent, false);
loading.id = intent.id;

return loading.promise;
}

/**
* @param {import('./types').NavigationInfo} info
* @param {import('./types').NavigationIntent} intent
* @param {string[]} chain
* @param {boolean} no_cache
* @param {{hash?: string, scroll: { x: number, y: number } | null, keepfocus: boolean}} [opts]
*/
async function _update(info, chain, no_cache, opts) {
async function _update(intent, chain, no_cache, opts) {
const current_token = (token = {});
let navigation_result = await _get_navigation_result(info, no_cache);
let navigation_result = await _get_navigation_result(intent, no_cache);

if (!navigation_result && info.url.pathname === location.pathname) {
if (!navigation_result && intent.url.pathname === location.pathname) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment

navigation_result = await _load_error({
status: 404,
error: new Error(`Not found: ${info.url.pathname}`),
url: info.url
error: new Error(`Not found: ${intent.url.pathname}`),
url: intent.url
});
}

if (!navigation_result) {
location.href = info.url.href;
location.href = intent.url.href;
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe navigate's last statements (ie. history manipulation) might run after this, depending on timing.

Suggested change
return;
return new Promise(() => {
/* never resolves */
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few places where we're doing a full page navigation — created a new traditional_navigation helper (open to better name suggestions) that encapsulates this behaviour, and replaced all location.href = ... occurrences with it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! traditional_navigation sounds alright, but I had to think to connect the dots. Consider navigate_natively, perform_native/browser_navigation or something that sounds like an action.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like native — have renamed it to native_navigation

}

Expand All @@ -215,17 +215,17 @@ export function create_client({ target, session, base, trailing_slash }) {
invalid.clear();

if (navigation_result.redirect) {
if (chain.length > 10 || chain.includes(info.url.pathname)) {
if (chain.length > 10 || chain.includes(intent.url.pathname)) {
navigation_result = await _load_error({
status: 500,
error: new Error('Redirect loop'),
url: info.url
url: intent.url
});
} else {
if (enabled) {
goto(new URL(navigation_result.redirect, info.url).href, {}, [
goto(new URL(navigation_result.redirect, intent.url).href, {}, [
...chain,
info.url.pathname
intent.url.pathname
]);
} else {
location.href = new URL(navigation_result.redirect, location.href).href;
Expand All @@ -236,7 +236,7 @@ export function create_client({ target, session, base, trailing_slash }) {
} else if (navigation_result.props?.page?.status >= 400) {
const updated = await stores.updated.check();
if (updated) {
location.href = info.url.href;
location.href = intent.url.href;
return;
}
}
Expand Down Expand Up @@ -281,7 +281,7 @@ export function create_client({ target, session, base, trailing_slash }) {
await tick();

if (autoscroll) {
const deep_linked = info.url.hash && document.getElementById(info.url.hash.slice(1));
const deep_linked = intent.url.hash && document.getElementById(intent.url.hash.slice(1));
if (scroll) {
scrollTo(scroll.x, scroll.y);
} else if (deep_linked) {
Expand Down Expand Up @@ -335,22 +335,22 @@ export function create_client({ target, session, base, trailing_slash }) {
}

/**
* @param {import('./types').NavigationInfo} info
* @param {import('./types').NavigationIntent} intent
* @param {boolean} no_cache
*/
async function _get_navigation_result(info, no_cache) {
if (loading.id === info.id && loading.promise) {
async function _get_navigation_result(intent, no_cache) {
if (loading.id === intent.id && loading.promise) {
return loading.promise;
}

for (let i = 0; i < info.routes.length; i += 1) {
const route = info.routes[i];
for (let i = 0; i < intent.routes.length; i += 1) {
const route = intent.routes[i];

// load code for subsequent routes immediately, if they are as
// likely to match the current path/query as the current one
let j = i + 1;
while (j < info.routes.length) {
const next = info.routes[j];
while (j < intent.routes.length) {
const next = intent.routes[j];
if (next[0].toString() === route[0].toString()) {
next[1].forEach((loader) => loader());
j += 1;
Expand All @@ -362,7 +362,7 @@ export function create_client({ target, session, base, trailing_slash }) {
const result = await _load(
{
route,
info
intent
},
no_cache
);
Expand Down Expand Up @@ -559,11 +559,9 @@ export function create_client({ target, session, base, trailing_slash }) {
* @param {import('./types').NavigationCandidate} selected
* @param {boolean} no_cache
*/
async function _load({ route, info: { url, path } }, no_cache) {
const key = url.pathname + url.search;

async function _load({ route, intent: { id, url, path } }, no_cache) {
if (!no_cache) {
const cached = cache.get(key);
const cached = cache.get(id);
if (cached) return cached;
}

Expand All @@ -574,7 +572,7 @@ export function create_client({ target, session, base, trailing_slash }) {
: {};

const changed = current.url && {
url: key !== current.url.pathname + current.url.search,
url: id !== current.url.pathname + current.url.search,
params: Object.keys(params).filter((key) => current.params[key] !== params[key]),
session: session_id !== current.session_id
};
Expand Down Expand Up @@ -801,19 +799,19 @@ export function create_client({ target, session, base, trailing_slash }) {
}

/** @param {URL} url */
function _parse(url) {
function get_navigation_intent(url) {
if (url.origin === location.origin && url.pathname.startsWith(base)) {
const path = decodeURI(url.pathname.slice(base.length) || '/');

/** @type {import('./types').NavigationInfo} */
const info = {
/** @type {import('./types').NavigationIntent} */
const intent = {
id: url.pathname + url.search,
routes: routes.filter(([pattern]) => pattern.test(path)),
url,
path
};

return info;
return intent;
}
}

Expand All @@ -835,21 +833,21 @@ export function create_client({ target, session, base, trailing_slash }) {
const from = current.url;
let should_block = false;

const intent = {
const navigation = {
from,
to: url,
cancel: () => (should_block = true)
};

callbacks.before_navigate.forEach((fn) => fn(intent));
callbacks.before_navigate.forEach((fn) => fn(navigation));

if (should_block) {
blocked();
return;
}

const info = _parse(url);
if (!info) {
const intent = get_navigation_intent(url);
if (!intent) {
location.href = url.href;
return new Promise(() => {
// never resolves
Expand All @@ -864,18 +862,18 @@ export function create_client({ target, session, base, trailing_slash }) {

const pathname = normalize_path(url.pathname, trailing_slash);

info.url = new URL(url.origin + pathname + url.search + url.hash);
intent.url = new URL(url.origin + pathname + url.search + url.hash);

const current_navigating_token = (navigating_token = {});

if (started) {
stores.navigating.set({
from: current.url,
to: info.url
to: intent.url
});
}

await _update(info, chain, false, {
await _update(intent, chain, false, {
scroll,
keepfocus
});
Expand All @@ -895,7 +893,7 @@ export function create_client({ target, session, base, trailing_slash }) {
if (details) {
const change = details.replaceState ? 0 : 1;
details.state[INDEX_KEY] = current_history_index += change;
history[details.replaceState ? 'replaceState' : 'pushState'](details.state, '', info.url);
history[details.replaceState ? 'replaceState' : 'pushState'](details.state, '', intent.url);
}
}

Expand Down Expand Up @@ -941,8 +939,8 @@ export function create_client({ target, session, base, trailing_slash }) {

if (!invalidating) {
invalidating = Promise.resolve().then(async () => {
const info = _parse(new URL(location.href));
if (info) await _update(info, [], true);
const intent = get_navigation_intent(new URL(location.href));
if (intent) await _update(intent, [], true);

invalidating = null;
});
Expand Down Expand Up @@ -977,13 +975,13 @@ export function create_client({ target, session, base, trailing_slash }) {
addEventListener('beforeunload', (e) => {
let should_block = false;

const intent = {
const navigation = {
from: current.url,
to: null,
cancel: () => (should_block = true)
};

callbacks.before_navigate.forEach((fn) => fn(intent));
callbacks.before_navigate.forEach((fn) => fn(navigation));

if (should_block) {
e.preventDefault();
Expand Down
18 changes: 15 additions & 3 deletions packages/kit/src/runtime/client/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,28 @@ export interface Client {
_start_router: () => void;
}

export type NavigationInfo = {
export type NavigationIntent = {
/**
* `url.pathname + url.search`
*/
id: string;
/**
* `url.pathname`, minus any `paths.base` prefix
*/
path: string;
/**
* The routes that could satisfy this navigation intent
*/
routes: CSRRoute[];
/**
* The destination URL
*/
url: URL;
path: string;
};

export type NavigationCandidate = {
route: CSRRoute;
info: NavigationInfo;
intent: NavigationIntent;
};

export type NavigationResult = {
Expand Down