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.

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

A function for getting the current load arguments #2979

Closed
Rich-Harris opened this issue Dec 3, 2021 · 25 comments
Closed

A function for getting the current load arguments #2979

Rich-Harris opened this issue Dec 3, 2021 · 25 comments
Milestone

Comments

@Rich-Harris
Copy link
Member

Describe the problem

fetch is great, but it's common to need a more powerful abstraction when loading data — a GraphQL client, or a custom api.get('endpoint') helper, or even just something that adds some logging around the fetch.

Right now that's somewhat cumbersome, because in order to make credentialled fetches during SSR, you need the instance of fetch that was passed into load. So you have to pick between these two monstrosities:

<script context="module">
  import * as api from '$lib/api';

  export async function load({ fetch }) {
    const data = api.get(fetch, 'endpoint');
    // ...
  }
</script>
<script context="module">
  export async function load({ stuff }) {
    const data = stuff.api.get('endpoint');
    // ...
  }
</script>

It gets worse if you need to pass page or session around for logging purposes.

The reason we have to do this is that the global fetch (supplied by node-fetch, in environments that don't natively support it) has no concept of credentials. A request for the /account page receives cookies; those cookies need to be forwarded when the page's load makes a request for the /account.json data that powers it, and the load-specific fetch argument makes that possible.

It would be nice if using an abstraction around fetch was less cumbersome.

Describe the proposed solution

Just like onMount registers a callback with the component that is currently initialising, we could introduce a function that makes the arguments to the current load invocation available to a function that gets called during that invocation. For example:

// src/lib/api.js
import { getLoadArguments } from '$app/load';

const base = import.meta.env.VITE_API_BASE_URL;

export async function get(endpoint) {
  const { fetch, page, session } = getLoadArguments();

  const start = Date.now();
  const url = `${base}/${endpoint}`;
  const res = await fetch(url);
  const name = session.user ? session.user.name : 'anonymous user';
  console.log(`fetched ${url} from ${page.path} for ${name} in ${Date.now() - start}ms`);

  return res;
}
<script context="module">
  import * as api from '$lib/api';

  export async function load() {
    const data = api.get('endpoint');
    // ...
  }
</script>

This function (hopefully we can come up with a better name than getLoadArguments) would only work during the invocation of a load; if called at any other time, it would throw an error.

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

@Rich-Harris

This comment has been minimized.

@Conduitry

This comment has been minimized.

@dummdidumm

This comment has been minimized.

@JoviDeCroock
Copy link

Hey all,

Just chiming in here as someone who has been trying to integrate urql with svelte-kit, first and foremost, awesome work already!

To give a little detail on how we work in urql up-to-date, there are probably several ways to tweak this to svelte-kit, we have an ssr plugin which hooks into our request-flow and saves all data that happens during a server-side render.
Example: urql.query(x) will be called on a page which will then be written to the ssr-plugin, when the ssr render completes we call ssrPlugin.extractData() and pass it on in a script tag that goes to the client and rehydrate from there.

In Next we've had to do this a bit more linear to this here, we have a load-function where we manually call the functions needed for the page we are rendering and then call ssrPlugin.extractData()

Just throwing these out here so we can have a wide variety of use-cases, thank you for starting this @Rich-Harris! 🙌

@Rich-Harris

This comment has been minimized.

@Rich-Harris
Copy link
Member Author

Going to hide the comments about removing stuff, it was a bad idea and shouldn't derail the issue.

@JoviDeCroock thanks for sharing those links. It definitely feels like this could help improve the ergonomics. It feels like the ssrExchange and ssrExtractData stuff should be unnecessary (fetch will already take care of that), but there could be some complexity I'm missing

@JoviDeCroock
Copy link

If the fetch succeeds at caching POST requests for GraphQL it should be possible, generally we do a lot of this due to us stringifying the query + variables and transforming it into a unique key for that result.

@Rich-Harris
Copy link
Member Author

It's designed to work that way — during SSR it will hash the request body...

${serialized_data
.map(({ url, body, json }) => {
let attributes = `type="application/json" data-type="svelte-data" data-url=${escape_html_attr(
url
)}`;
if (body) attributes += ` data-body="${hash(body)}"`;
return `<script ${attributes}>${json}</script>`;
})
.join('\n\n\t')}

...then if a fetch is attempted client-side with the same body it will use the serialized data (if no match, it will fall back to the network, so it's resilient):

function initial_fetch(resource, opts) {
const url = typeof resource === 'string' ? resource : resource.url;
let selector = `script[data-type="svelte-data"][data-url=${JSON.stringify(url)}]`;
if (opts && typeof opts.body === 'string') {
selector += `[data-body="${hash(opts.body)}"]`;
}
const script = document.querySelector(selector);
if (script && script.textContent) {
const { body, ...init } = JSON.parse(script.textContent);
return Promise.resolve(new Response(body, init));
}
return fetch(resource, opts);
}

@Conduitry
Copy link
Member

This is not well thought-out, but is there any reasonable way to combine this feature with making the $app/stores exports magically work inside the load function?

I have various helper functions in my app that I need to sometimes call within a load function and sometimes within the main body of a component, and it currently takes a little bit of fiddling to allow them to either be passed in the session from load or to get the $session store from $app/stores.

With the changes proposed in this issue, I couldn't quite get rid of these extra hoops, because these functions still won't know whether to use the new $app/load or to use $app/stores. Is there anything that makes sense here?

@Conduitry
Copy link
Member

Conduitry commented Jan 10, 2022

I was just thinking about this again and ... how is this feature actually going to work on the server? Doesn't this basically require some sort of continuation-local storage system? If we could let anyone grab these arguments at any time, they wouldn't even need to be arguments - they could just be globals.

Are we going to say that getLoadArguments needs to be called synchronously after the start of load? Does the JS spec even say that any code that's run in an async function before the first await will be synchronously run right when you call the function?

@Rich-Harris
Copy link
Member Author

yes, the async-ness of a function has no bearing on the matter — this will always log 'before', 'fooing', 'after':

async function foo() {
  console.log('fooing');
}

function test() {
  console.log('before');
  foo();
  console.log('after');
}

test();

Which means that figuring out which load function is running involves no magic:

export function getLoadArguments() {
  return current_load_arguments;
}

function navigate() {
  current_load_arguments = { url, params, session, stuff, fetch };
  const promise = load.call(null, current_load_arguments);
  current_load_arguments = null;
}

@Conduitry
Copy link
Member

Gotcha, okay, so we'll just need to document that getLoadArguments will need to be called synchronously after the start of load. The main place I see this causing a problem is when someone wants to run two network requests in series during load - the second one won't have access to the load arguments.

@dummdidumm
Copy link
Member

Would there be benefit in making the arguments available during the whole load method, resetting it only after the promise has resolved. Or does this not work because the load arguments could become stale/outdated or sth?

@Conduitry
Copy link
Member

It doesn't work because there might be multiple requests being served concurrently by the server - the same reason that they are arguments to load to begin with rather than being some sort of globals.

@icalvin102
Copy link
Contributor

icalvin102 commented Feb 5, 2022

Hi there,

I would really like this proposal to work but I think the fact that getLoadArguments can only be called at the start of the load function is a constraint that makes it pretty useless in many real world use cases as it only works with single or multiple independent requests. It's no longer safe to use getLoadArguments as soon as requests depend on each other because api requests outside of the load function may be called during that time.

I would like to propose an other solution that introduces some kind of hook that makes it possible to customize the way load is called.

function loadHook(node: BranchNode, load_input: LoadInput): LoadOutput {
  const api = API.createWithLoadArguments(load_input);
  
  return node.module.load(load_input, api);
}

The load function now receives a custom api argument.

async function load({page}, api) {
  const group = await api.getGroup(page.params.groupid);
  const members = await api.followLink(group, 'members');
  
  return {
    props: { group, members }
  }
}

The src/runtime/client/renderer.js#L676 would have to be changed as follows.

- const loaded = await module.load.call(null, load_input);
+ const loaded = await (loadHook ? loadHook(node, load_input) : module.load.call(null, load_input));

This solution is of cause not limited to the api example but could be used for logging or transformation of the LoadInput and LoadOutput as well.
The access to the BranchNode would allow fine grained control over the dependencies of the load function.

@jycouet
Copy link
Contributor

jycouet commented Apr 6, 2022

@Conduitry, load & stores are already magic.

1/ In SSR, load fetch the data
2/ load is executed in the client and reuse the result of the SSR (not a second fetch, thx to SvelteKit).
3/ At this time you can fill stores with your data
=> You can check it out here: KitQL - Demo 1

Not that nice to pass the fetch, but minimal amount of code 👍


I'll be very interested in adding getLoadArguments to the generating part of KitQL and be able to write:

import { KQL_AllContinents } from '$lib/graphql/_kitql/graphqlStores';

export async function load() {
	await KQL_AllContinents.queryLoad(); // Work in SSR & Client mode
	// await KQL_another-operation-filling-another-store.queryLoad();
	return {};
}

And getting rid of the fetch part.

@georgecrawford
Copy link
Contributor

I've just posted this discussion which is along the same lines of the thinking here - how to reduce the cumbersome boilerplate in using SvelteKit's fetch implementation when you need to add headers, handle responses, add logging, etc. Ideally I'd like a solution which works in load() (both in SSR and in the browser) and also in the component lifecycle as well (as @Conduitry mentioned above).

I did have one question in response to this from @Rich-Harris:

This function (hopefully we can come up with a better name than getLoadArguments) would only work during the invocation of a load; if called at any other time, it would throw an error.

Can I confirm that, outside of a load() invocation (i.e. in a standard component, perhaps when a form is submitted or a button is pressed), there is no advantage to using the 'special' SvelteKit load() implementation, and we should use window.fetch instead? If so, it'd be great to document that clearly.

@madeleineostoja
Copy link

Someone else can feel free to correct me, but afaik the only thing sveltekit’s provided fetch does is hydrate the response fetched during load on the server, so there would be no reason to use it outside of that context (if it even works outside of that context)

@Conduitry
Copy link
Member

#4625 is one thing the client-side load fetch doesn't do today but which we'd like it to do.

@jaclas
Copy link

jaclas commented Jun 30, 2022

(hopefully we can come up with a better name than getLoadArguments)

maybe getLoadInput()?

Is the work on this goal advanced? Because I don't know whether to build access to the external API with tricks or wait for the completion of this track?

By the way, I wanted to ask about this:

<script context="module">
  export async function load({ stuff }) {
    const data = stuff.api.get('endpoint');
    // ...
  }
</script>

Rich in the initiating post gave two examples that he called "monstrosities", the first I understand, but the one above does not. Where in stuff does the api come from?

@georgecrawford
Copy link
Contributor

@jaclas I'm currently using stuff in the above-mentioned discussion: #5173. Rich's proposal would make this much cleaner.

@Rich-Harris
Copy link
Member Author

We realised last night that this feature is unnecessary — you can do everything with a higher order function:

export const load = extra_fancy_load(({ client }) => {
  return client.get_stuff();
});

This is much less magical, and doesn't come with any caveats around timing.

The one major caveat is that extra_fancy_load would most likely want to destructure the event...

return fn.call(null, { ...event, client });

...which would have the effect of triggering getters for url and params that would mark them as dependencies, incorrectly. It's easy enough to work around, but it'd be nice to have some better patterns for that.

@gterras
Copy link

gterras commented Nov 21, 2022

We realised last night that this feature is unnecessary — you can do everything with a higher order function:

export const load = extra_fancy_load(({ client }) => {
  return client.get_stuff();
});

Could you please elaborate a bit more on this technique (like how to define and type extra_fancy_load, and also why is it less ugly that the inital examples)? I couldn't find anything on the docs.

@icalvin102
Copy link
Contributor

icalvin102 commented Nov 22, 2022

@gterras a simple implementation of extra_fancy_load could look something like this.

const extra_fancy_load = (loadFunction) => {
  return async function load(args) {
    const client = API.createClientWithLoadArguments(args);
    return await loadFunction({...args, client});
  }
}

The beauty of this (compared to the initial examples) is that the load arguments don't need to be passed around in the background and it has no side effects.

And it is much more composeable.

Example:

const logging_load = (loadFunction) => {
  return async function load(args) {
    const start = Date.now();
    const result = await loadFunction(args);
    console.log(`Loaded ${args.url.href} in ${Date.now() - start}ms`);
    return result;
  }
}

Simple usage:

export const load = logging_load(async ({fetch}) => {
  const res = await fetch('/some/data');
  const json = await res.json();
  return json;
})

Or compose it with extra_fancy_load:

export const load = logging_load(extra_fancy_load(({ client }) => {
  return client.get_stuff();
}))

@gterras
Copy link

gterras commented Nov 22, 2022

@gterras a simple implementation of extra_fancy_load could look something like this.

Thanks for this, very interesting this should be mentioned in the docs.

What would be the proper way to TS type extra_fancy_load (say when a client prop is added to the LoadEvent, and extra_fancy_load is meant to be imported in each load function, whether they are any declination of Load like LayoutServerLoad or such)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants