kit icon indicating copy to clipboard operation
kit copied to clipboard

invalidate() method timeout

Open CaptainCodeman opened this issue 3 years ago • 7 comments

Describe the problem

If invalidate() is called and the server generates an error, it appears the client is directed to an error page with the appropriate details. ✅

But if invalidate() is called and there is simply no response, then what? Right now it appears happy to wait forever (where "forever" is at least the 5 minutes I waited). This could lead to inconsistent state if not handled.

Should we be expected to add our own timeout mechanism or could a default timeout option be set or invalidate() allow a timeout parameter that could generate an error on the client?

Describe the proposed solution

Either a global timeout or a per-invalidate timeout, that will generate a timeout error on the client (or a combination of timeout and retry mechanism).

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

CaptainCodeman avatar Aug 25 '22 16:08 CaptainCodeman

This plus #5305, plus a separate conversation about allowing non-URL arguments to invalidate (useful for client-side state management etc) all point to the need for a bit of a rethink of the API, so I'm marking this as a breaking change

Rich-Harris avatar Aug 27 '22 15:08 Rich-Harris

Allowing a timeout only on invalidate() seems like a weird API if we don't also allow it on regular client-side navigations or on programmatic goto() calls - which seems to indicate to me that the concept of load timeouts should live somewhere else, separate from any of these specific reasons it should be called, but I don't know where that should be.

Conduitry avatar Aug 27 '22 17:08 Conduitry

I was wondering if such an API is really necessary. A user could do something like await Promise.race([new Promise((_, reject) => setTimeout() => reject('timeout'), 1000), invalidate('..')]) to get the behavior themselves. But then I realized that this would not abort the invalidation if there's no other navigation/invalidation in the meantime (if there was, SvelteKit would know that is outdated and ignore the result), so if it would resolve some time later that could be confusing.

I was also thinking about what @Conduitry said but I couldn't come up with a common place for this other than the config file which is too broad/not the right place. A thought I had was adding an input method to load functions that signal that things time out after some time, but that's not good API either. Ultimately I feel this is fine to be added on both invalidate and goto

dummdidumm avatar Oct 28 '22 13:10 dummdidumm

Coming back to this again, my workaround with Promise.race does work if you do

await Promise.race([
  new Promise((_, reject) => setTimeout() => { goto($page.url); reject(); }, 1000),
  invalidate('..')
]);

This could be packed up into a little helper function. I'm inclined to move this post 1.0 to wait for more people expressing desire to have something like this built in where such a helper function (which also provides way more control, because you can do more than timeout) wouldn't suffice.

Other ways this could be solved:

  • some kind of abortLoading() function which stops any invalidation/navigation that currently takes place
  • passing in an abort signal to to invalidate/goto which you call when appropriate

Implementation-wise I think abortLoading is probably easier to implement

dummdidumm avatar Oct 31 '22 09:10 dummdidumm

Whether we do it now or later, I think the framework should have support built-in for it. And I think it should happen automatically and not require any special APIs. It should probably be configurable both globally and for an individual call

benmccann avatar Nov 07 '22 15:11 benmccann

I still don't see why we would want this on programmatic invalidate()/goto() but not have a way to specify it for intercepted link clicks (or for SSR'd content, for that matter). What makes the programmatic calls special that we want to be able to specify a timeout for them, but not the other times the framework is calling load() automatically?

What would happen if we allowed a export const timeout = 1000; in +page.js? Is there a reason to want different timeouts in the four different situations that load() might be getting called?

If we want to be code-over-configuration about this (at least at first) what about a wrapper function around the whole load() function that Promise.races it with a given timeout that can vary between SSR and CSR if the user wants? Higher-order functions that augment the load() function (say, to inject additional arguments) are already a pattern we're recommending. It'd be nice to eventually let people specify those on all of their load() functions at once in a somewhat more DRY way. If we turn the problem in this issue into an augmenting-load() problem, we could eventually solve both at once.

Conduitry avatar Nov 07 '22 23:11 Conduitry

Everything @Conduitry is saying makes sense to me — this isn't an invalidate-specific issue, it belongs to the load itself. A higher-order function is a good solution.

A more declarative approach like export const timeout would be a nice addition, though it could take two different forms:

  • pages inherit timeout configuration from parent layouts, as happens with other page options like prerender
  • you have to declare export const timeout for the load function you're exporting from the same module

In a certain sense the second makes more sense — src/routes/potato/+page.js shouldn't really have any say over how long is too long to wait for the load function in src/routes/+layout.server.js. But the first would be a hell of a lot easier — set it once in your root layout, and it applies across the entire app, and it works the way you'd expect if you've used the other page options.

Either way it would be a non-breaking change, and the userland solution (higher order functions) can be used today, so I no longer think this needs to be solved pre-1.0.

Rich-Harris avatar Nov 15 '22 22:11 Rich-Harris