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

feat: allow for fine grained invalidation of search params #11066

Conversation

paoloricciuti
Copy link
Member

@paoloricciuti paoloricciuti commented Nov 17, 2023

This PR basically allow for sveltekit users to have fine grained control over the access to searchParams. This means that if inside a load function i access url.searchParams.get("test") and nothing more that load function will actually only rerun when the test search params changes (so not if the url changes and the search param is the same, not if another search param changes).

There's an issue about this #8403 and i've talked both with Simon and Ben before going with this PR.

Technically this is a breaking change (in case someone was relying on this behavior to invalidate the route by accessing a search param). We could in theory put this behind a settings and eventually do the breaking change in kit 2.0, what do you think?

I've added the tests only for get but in theory there could be tests to check for has, getAll and the fact that every other thing accessed (like size, entries, values) should invalidate the whole url. Let me know if you want me to add also that.

I think it would be useful to update the docs too in this PR, if you think otherwise I'll continue working on this in an HR or so, so stop me now if you want! I added the docs too.

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. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Nov 17, 2023

🦋 Changeset detected

Latest commit: ff8b4bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

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

paoloricciuti and others added 2 commits November 23, 2023 11:10
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
@paoloricciuti
Copy link
Member Author

Looks good!
One thing I'm not quite sure about: Will this solves #8403 (comment), too? i.e. the URL changes, but since you're only listening to search params the load function should not rerun when the search param does not change. Then again, that's probably wrong in other circumstances, since you may have the same search param on a different page which does mean something different, and as such it should rerun? Then again, if the value does not change, and you only use the value to request data, that's correct anyway? Mhm ...

Yeah this solves that problem too and as you've said if it's dependent only on that searchParam if it's not changing you don't need to rerun the load function.

@paoloricciuti
Copy link
Member Author

@dummdidumm gentle ping that this is technically ready...do it at your own pace since i know there's a lot of stuff going on, just wanted to remind since from outside it seems CI is failing because vercel is not authorized to deploy but all the rest of the stuff is green 😄

* @returns {import('./types.js').Client}
*/
export function create_client(app, target) {
export function create_client(app, target, fine_grained_search_params_invalidation) {
Copy link
Contributor

@gtm-nayan gtm-nayan Nov 30, 2023

Choose a reason for hiding this comment

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

I don't see a reason to pass this into create_client as a param. This could be done using define and that'd reduce the complexity of the config quite a bit on the server side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think i've ever heard of define...do you have a link to some resource i can look into?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh i think now i realized it's from vite...exploring this a bit

@paoloricciuti
Copy link
Member Author

How is the deploying failing now? I just added a couple of letters in the markdown 😄

@dummdidumm
Copy link
Member

Sorry the back and forth - we decided to make this part of SvelteKit 2.0 (which we want to start working on very soon), so you can remove the config option again 😅

@paoloricciuti
Copy link
Member Author

Sorry the back and forth - we decided to make this part of SvelteKit 2.0 (which we want to start working on very soon), so you can remove the config option again 😅

No problem I will revert those commits back, just a bit sad because I needed this feature but it's fine as long as sooner or later we can get that 😄

@Rich-Harris
Copy link
Member

Thanks — made a few changes in #11258 (couldn't commit directly to this branch) so I'll close this in favour of that

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

Successfully merging this pull request may close these issues.

6 participants