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

invalidate() method timeout #6274

Open
CaptainCodeman opened this issue Aug 25, 2022 · 7 comments
Open

invalidate() method timeout #6274

CaptainCodeman opened this issue Aug 25, 2022 · 7 comments
Labels
feature / enhancement New feature or request
Milestone

Comments

@CaptainCodeman
Copy link
Contributor

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 CaptainCodeman changed the title Invalidate() method timeout invalidate() method timeout Aug 25, 2022
@Rich-Harris Rich-Harris added the feature / enhancement New feature or request label Aug 25, 2022
@Rich-Harris
Copy link
Member

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 Rich-Harris added this to the 1.0 milestone Aug 27, 2022
@Conduitry
Copy link
Member

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.

dummdidumm pushed a commit that referenced this issue Sep 1, 2022
Closes #6354.

Related to #6489, #6274 and #5305 

Co-authored-by: Rich Harris <hello@rich-harris.dev>
Co-authored-by: icalvin102 <calvin@icalvin.de>
Co-authored-by: Immanuel Calvin Herchenbach <40042006+icalvin102@users.noreply.github.com>
@dummdidumm
Copy link
Member

dummdidumm commented Oct 28, 2022

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
Copy link
Member

dummdidumm commented Oct 31, 2022

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

@benmccann
Copy link
Member

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

@Conduitry
Copy link
Member

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.

@Rich-Harris
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants