Skip to content

[breaking] rename context param of load to stuff #2439

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

Merged
merged 8 commits into from
Sep 22, 2021
Merged

[breaking] rename context param of load to stuff #2439

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f12f013
[breaking] rename context param of load to stuff
Sep 16, 2021
33eda8a
lint
Sep 16, 2021
fc17ce3
Log warning when someone uses context
Sep 17, 2021
84e94ff
.js
Sep 17, 2021
3a570eb
remove dev check, throw instead of log
Sep 20, 2021
476d54d
Merge branch 'master' into stuff
Sep 20, 2021
d1a6573
format
Sep 20, 2021
997d795
remove todo
Sep 21, 2021
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
5 changes: 5 additions & 0 deletions .changeset/neat-garlics-buy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

**breaking change** the "context" parameter of the load function was renamed to "stuff"
20 changes: 10 additions & 10 deletions documentation/docs/03-loading.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ A component that defines a page or a layout can export a `load` function that ru

export interface LoadInput<
PageParams extends Record<string, string> = Record<string, string>,
Context extends Record<string, any> = Record<string, any>,
Stuff extends Record<string, any> = Record<string, any>,
Session = any
> {
page: {
Expand All @@ -21,18 +21,18 @@ export interface LoadInput<
};
fetch(info: RequestInfo, init?: RequestInit): Promise<Response>;
session: Session;
context: Context;
stuff: Stuff;
}

export interface LoadOutput<
Props extends Record<string, any> = Record<string, any>,
Context extends Record<string, any> = Record<string, any>
Stuff extends Record<string, any> = Record<string, any>
> {
status?: number;
error?: string | Error;
redirect?: string;
props?: Props;
context?: Context;
stuff?: Stuff;
maxage?: number;
}
```
Expand All @@ -44,7 +44,7 @@ Our example blog page might contain a `load` function like the following:
/**
* @type {import('@sveltejs/kit').Load}
*/
export async function load({ page, fetch, session, context }) {
export async function load({ page, fetch, session, stuff }) {
const url = `/blog/${page.params.slug}.json`;
const res = await fetch(url);

Expand Down Expand Up @@ -90,7 +90,7 @@ It is recommended that you not store pre-request state in global variables, but

### Input

The `load` function receives an object containing four fields — `page`, `fetch`, `session` and `context`. The `load` function is reactive, and will re-run when its parameters change, but only if they are used in the function. Specifically, if `page.query`, `page.path`, `session`, or `context` are used in the function, they will be re-run whenever their value changes. Note that destructuring parameters in the function declaration is enough to count as using them. In the example above, the `load({ page, fetch, session, context })` function will re-run every time `session` or `context` is changed, even though they are not used in the body of the function. If it was re-written as `load({ page, fetch })`, then it would only re-run when `page.params.slug` changes. The same reactivity applies to `page.params`, but only to the params actually used in the function. If `page.params.foo` changes, the example above would not re-run, because it did not access `page.params.foo`, only `page.params.slug`.
The `load` function receives an object containing four fields — `page`, `fetch`, `session` and `stuff`. The `load` function is reactive, and will re-run when its parameters change, but only if they are used in the function. Specifically, if `page.query`, `page.path`, `session`, or `stuff` are used in the function, they will be re-run whenever their value changes. Note that destructuring parameters in the function declaration is enough to count as using them. In the example above, the `load({ page, fetch, session, stuff })` function will re-run every time `session` or `stuff` is changed, even though they are not used in the body of the function. If it was re-written as `load({ page, fetch })`, then it would only re-run when `page.params.slug` changes. The same reactivity applies to `page.params`, but only to the params actually used in the function. If `page.params.foo` changes, the example above would not re-run, because it did not access `page.params.foo`, only `page.params.slug`.

#### page

Expand All @@ -117,9 +117,9 @@ So if the example above was `src/routes/blog/[slug].svelte` and the URL was `htt

`session` can be used to pass data from the server related to the current request, e.g. the current user. By default it is `undefined`. See [`getSession`](#hooks-getsession) to learn how to use it.

#### context
#### stuff

`context` is passed from layout components to child layouts and page components. For the root `__layout.svelte` component, it is equal to `{}`, but if that component's `load` function returns an object with a `context` property, it will be available to subsequent `load` functions.
`stuff` is passed from layout components to child layouts and page components and can be filled with anything else you need to make available. For the root `__layout.svelte` component, it is equal to `{}`, but if that component's `load` function returns an object with a `stuff` property, it will be available to subsequent `load` functions.

### Output

Expand Down Expand Up @@ -147,8 +147,8 @@ This only applies to page components, _not_ layout components.

If the `load` function returns a `props` object, the props will be passed to the component when it is rendered.

#### context
#### stuff

This will be merged with any existing `context` and passed to the `load` functions of subsequent layout and page components.
This will be merged with any existing `stuff` and passed to the `load` functions of subsequent layout and page components.

This only applies to layout components, _not_ page components.
2 changes: 1 addition & 1 deletion documentation/migrating/04-pages.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Any files you previously imported from directories in `src/node_modules` will ne

As before, pages and layout components can export a function that allows data to be loaded before rendering takes place.

This function has been renamed from `preload` to [`load`](/docs#loading), and its API has changed. Instead of two arguments — `page` and `session` — there is a single argument that includes both, along with `fetch` (which replaces `this.fetch`) and a new `context` object.
This function has been renamed from `preload` to [`load`](/docs#loading), and its API has changed. Instead of two arguments — `page` and `session` — there is a single argument that includes both, along with `fetch` (which replaces `this.fetch`) and a new `stuff` object.

There is no more `this` object, and consequently no `this.fetch`, `this.error` or `this.redirect`. Instead of returning props directly, `load` now returns an object that _contains_ `props`, alongside various other things.

Expand Down
54 changes: 27 additions & 27 deletions packages/kit/src/runtime/client/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class Renderer {
const branch = [];

/** @type {Record<string, any>} */
let context = {};
let stuff = {};

/** @type {import('./types').NavigationResult | undefined} */
let result;
Expand All @@ -153,7 +153,7 @@ export class Renderer {
const node = await this._load_node({
module: await nodes[i],
page,
context,
stuff,
status: is_leaf ? status : undefined,
error: is_leaf ? error : undefined
});
Expand All @@ -169,10 +169,10 @@ export class Renderer {
path: page.path,
query: page.query
};
} else if (node.loaded.context) {
context = {
...context,
...node.loaded.context
} else if (node.loaded.stuff) {
stuff = {
...stuff,
...node.loaded.stuff
};
}
}
Expand Down Expand Up @@ -451,11 +451,11 @@ export class Renderer {
* error?: Error;
* module: CSRComponent;
* page: import('types/page').Page;
* context: Record<string, any>;
* stuff: Record<string, any>;
* }} options
* @returns
*/
async _load_node({ status, error, module, page, context }) {
async _load_node({ status, error, module, page, stuff }) {
/** @type {import('./types').BranchNode} */
const node = {
module,
Expand All @@ -464,11 +464,11 @@ export class Renderer {
path: false,
query: false,
session: false,
context: false,
stuff: false,
dependencies: []
},
loaded: null,
context
stuff
};

/** @type {Record<string, string>} */
Expand Down Expand Up @@ -506,9 +506,9 @@ export class Renderer {
node.uses.session = true;
return session;
},
get context() {
node.uses.context = true;
return { ...context };
get stuff() {
node.uses.stuff = true;
return { ...stuff };
},
fetch(resource, info) {
const url = typeof resource === 'string' ? resource : resource.url;
Expand All @@ -530,7 +530,7 @@ export class Renderer {
if (!loaded) return;

node.loaded = normalize(loaded);
if (node.loaded.context) node.context = node.loaded.context;
if (node.loaded.stuff) node.stuff = node.loaded.stuff;
}

return node;
Expand Down Expand Up @@ -569,8 +569,8 @@ export class Renderer {
let branch = [];

/** @type {Record<string, any>} */
let context = {};
let context_changed = false;
let stuff = {};
let stuff_changed = false;

/** @type {number | undefined} */
let status = 200;
Expand Down Expand Up @@ -599,13 +599,13 @@ export class Renderer {
(changed.query && previous.uses.query) ||
(changed.session && previous.uses.session) ||
previous.uses.dependencies.some((dep) => this.invalid.has(dep)) ||
(context_changed && previous.uses.context);
(stuff_changed && previous.uses.stuff);

if (changed_since_last_render) {
node = await this._load_node({
module,
page,
context
stuff
});

const is_leaf = i === a.length - 1;
Expand All @@ -624,8 +624,8 @@ export class Renderer {
};
}

if (node.loaded.context) {
context_changed = true;
if (node.loaded.stuff) {
stuff_changed = true;
}
} else if (is_leaf && module.load) {
// if the leaf node has a `load` function
Expand Down Expand Up @@ -658,7 +658,7 @@ export class Renderer {
error,
module: await b[i](),
page,
context: node_loaded.context
stuff: node_loaded.stuff
});

if (error_loaded && error_loaded.loaded && error_loaded.loaded.error) {
Expand All @@ -680,10 +680,10 @@ export class Renderer {
query
});
} else {
if (node && node.loaded && node.loaded.context) {
context = {
...context,
...node.loaded.context
if (node && node.loaded && node.loaded.stuff) {
stuff = {
...stuff,
...node.loaded.stuff
};
}

Expand Down Expand Up @@ -713,7 +713,7 @@ export class Renderer {
const node = await this._load_node({
module: await this.fallback[0],
page,
context: {}
stuff: {}
});

const branch = [
Expand All @@ -723,7 +723,7 @@ export class Renderer {
error,
module: await this.fallback[1],
page,
context: (node && node.loaded && node.loaded.context) || {}
stuff: (node && node.loaded && node.loaded.stuff) || {}
})
];

Expand Down
4 changes: 2 additions & 2 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,10 +28,10 @@ export type BranchNode = {
path: boolean;
query: boolean;
session: boolean;
context: boolean;
stuff: boolean;
dependencies: string[];
};
context: Record<string, any>;
stuff: Record<string, any>;
};

export type NavigationState = {
Expand Down
9 changes: 9 additions & 0 deletions packages/kit/src/runtime/load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { dev } from './app/env.js';

/**
* @param {import('types/page').LoadOutput} loaded
* @returns {import('types/internal').NormalizedLoadOutput}
Expand Down Expand Up @@ -52,5 +54,12 @@ export function normalize(loaded) {
}
}

if (dev && /** @type {any} */ (loaded).context) {
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 the warning. Using dev from ./app/env here, which wasn't used anywhere else yet (only for user-facing-code), hope that doesn't have any bad implications.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, it does apperently, tests are failing now because env is not set in tests. What now?

Copy link
Member

Choose a reason for hiding this comment

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

Would context still work in prod? If not I think we should make a hard error here.

But regarding the error, I believe it's because import.meta.env is non-standard in nodejs so it would resolve to undefined, hence the error. A workaround is to shim it with import.meta.env = { /* ... */ } at the top of this file, though we need to make sure we do that for tests only. Using process.env.NODE_ENV may be fine for this as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect it to be defined in the integration tests. I think the issue is around this being unit tested. Rather than modify this to shim it, I wonder if we could move the check to another location that's called in integration tests, but not unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This sounds wrong to me. I picked this method as it seems the central location to validating the load response. Also we probably don't have this check in for long, we could remove it at 1.0 and before that shimming it sounds like a pragmatic solution in the meantime

Copy link
Member

Choose a reason for hiding this comment

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

Ok. That makes sense

console.warn(
'You are returning "context" from a load function. ' +
'"context" was renamed to "stuff", please adjust your code accordingly.'
);
}

return /** @type {import('types/internal').NormalizedLoadOutput} */ (loaded);
}
8 changes: 4 additions & 4 deletions packages/kit/src/runtime/server/page/load_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const s = JSON.stringify;
* page: import('types/page').Page;
* node: import('types/internal').SSRNode;
* $session: any;
* context: Record<string, any>;
* stuff: Record<string, any>;
* prerender_enabled: boolean;
* is_leaf: boolean;
* is_error: boolean;
Expand All @@ -29,7 +29,7 @@ export async function load_node({
page,
node,
$session,
context,
stuff,
prerender_enabled,
is_leaf,
is_error,
Expand Down Expand Up @@ -269,7 +269,7 @@ export async function load_node({
})
);
},
context: { ...context }
stuff: { ...stuff }
};

if (is_error) {
Expand All @@ -293,7 +293,7 @@ export async function load_node({
return {
node,
loaded: normalize(loaded),
context: loaded.context || context,
stuff: loaded.stuff || stuff,
fetched,
set_cookie_headers,
uses_credentials
Expand Down
14 changes: 7 additions & 7 deletions packages/kit/src/runtime/server/page/respond.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export async function respond(opts) {
let set_cookie_headers = [];

ssr: if (page_config.ssr) {
let context = {};
let stuff = {};

for (let i = 0; i < nodes.length; i += 1) {
const node = nodes[i];
Expand All @@ -86,7 +86,7 @@ export async function respond(opts) {
loaded = await load_node({
...opts,
node,
context,
stuff,
prerender_enabled: is_prerender_enabled(options, node, state),
is_leaf: i === nodes.length - 1,
is_error: false
Expand Down Expand Up @@ -141,7 +141,7 @@ export async function respond(opts) {
const error_loaded = /** @type {import('./types').Loaded} */ (await load_node({
...opts,
node: error_node,
context: node_loaded.context,
stuff: node_loaded.stuff,
prerender_enabled: is_prerender_enabled(options, error_node, state),
is_leaf: false,
is_error: true,
Expand Down Expand Up @@ -183,11 +183,11 @@ export async function respond(opts) {
}
}

if (loaded && loaded.loaded.context) {
if (loaded && loaded.loaded.stuff) {
// TODO come up with better names for stuff
context = {
...context,
...loaded.loaded.context
stuff = {
...stuff,
...loaded.loaded.stuff
};
}
}
Expand Down
4 changes: 2 additions & 2 deletions packages/kit/src/runtime/server/page/respond_with_error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export async function respond_with_error({ request, options, state, $session, st
page,
node: default_layout,
$session,
context: {},
stuff: {},
prerender_enabled: is_prerender_enabled(options, default_error, state),
is_leaf: false,
is_error: false
Expand All @@ -55,7 +55,7 @@ export async function respond_with_error({ request, options, state, $session, st
page,
node: default_error,
$session,
context: loaded ? loaded.context : {},
stuff: loaded ? loaded.stuff : {},
prerender_enabled: is_prerender_enabled(options, default_error, state),
is_leaf: false,
is_error: true,
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/src/runtime/server/page/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { NormalizedLoadOutput, SSRNode } from 'types/internal';
export type Loaded = {
node: SSRNode;
loaded: NormalizedLoadOutput;
context: Record<string, any>;
stuff: Record<string, any>;
fetched: Array<{ url: string; body: string; json: string }>;
set_cookie_headers: string[];
uses_credentials: boolean;
Expand Down
Loading