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

[doc] more information on depends and invalidate #6489

Closed
wants to merge 3 commits into from

Conversation

icalvin102
Copy link
Contributor

@icalvin102 icalvin102 commented Aug 31, 2022

depends and invalidate are not documented enough.
Related issues (most recently #6354) and a lot of questions on the Svelte discord server show that there is some confusion about their usage.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2022

⚠️ No Changeset found

Latest commit: 8ee88e7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@lukaszpolowczyk
Copy link
Contributor

lukaszpolowczyk commented Aug 31, 2022

@icalvin102 I have a general question about depends and invalidate.

E.g. such code

/** @type {import('./$types').PageLoad} */
export async function load({ fetch }) {
  const url = `https://cms.example.com/articles.json`;
  const url2 = `https://cms.example.com/articles2.json`;
  const response = await fetch(url);
  const response2 = await fetch(url2);
 
  return {
    articles: await response.json(),
    articles2: await response2.json()
  };

And then invalidate:

invalidate((url) => url===`https://cms.example.com/articles.json`)`

Question: does it execute the entire load after invalidate?
Or does it execute the entire load, but bypassing the await fetch(url2)?

Because it is clearly stated in invalidate that only url is out of date.
From this it follows that url2 is up-to-date, so there is no need to perform await fetch(url2).
Is this how it works?
In this situation, does await fetch(url2) return the data from the cache, instead of executing itself?

It seems to me that this would make sense.
Just not sure where such a cache would be stored.


Oh, and it's hard for custom depends to handle cache.
So that you would have to somehow bind, for example, the api.client.get('/foo') function to the id used in depends() to be able to reach the cache that way.
Surely that wouldn't be so easy anymore?

Copy link
Member

@Rich-Harris Rich-Harris left a comment

Choose a reason for hiding this comment

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

thank you! couple of small comments, plus some notes about improvements to the invalidate API that i think would be worth sneaking in ahead of this

documentation/docs/05-load.md Show resolved Hide resolved
documentation/docs/05-load.md Outdated Show resolved Hide resolved
packages/kit/types/ambient.d.ts Show resolved Hide resolved
packages/kit/types/ambient.d.ts Show resolved Hide resolved
packages/kit/types/ambient.d.ts Outdated Show resolved Hide resolved
@Rich-Harris Rich-Harris mentioned this pull request Aug 31, 2022
5 tasks
Co-authored-by: Rich Harris <hello@rich-harris.dev>
@icalvin102
Copy link
Contributor Author

@lukaszpolowczyk invalidate causes the entire load function to rerun. I does not perform any magic inside of the load function.

Automatic caching of individual resources in load would be really difficult because some resources may depend on each other.

However giving the user a way to determine which resource should be fetched again would theoretically be possible if load would provide something like isInvalidating(url)

export async function load({ fetch, isInvalidating }) {
  const url = `https://cms.example.com/articles.json`;
  const url2 = `https://cms.example.com/articles2.json`;
  let response, response2;
  
  if(isInvalidating(url)) {
    response = await fetch(url);
    cache.set(url, response);
  } else {
    response = cache.get(url);
  }
  
  if(isInvalidating(url2)) {
    response2 = await fetch(url);
    cache.set(url2, response2);
  } else {
    response2 = cache.get(url2);
  }
 
  return {
    articles: await response.json(),
    articles2: await response2.json()
  };
}

But this feature does currently not exist. Might be worth a feature request.

@lukaszpolowczyk
Copy link
Contributor

But this feature does currently not exist. Might be worth a feature request.

@icalvin102 I added - #6495

@Rich-Harris
Copy link
Member

incorporated this branch into #6493, so I'll close this — thanks!

@Rich-Harris Rich-Harris closed this Sep 1, 2022
dummdidumm pushed a commit that referenced this pull request 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants