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

fix: recurse into $derived for ownership validation #15166

Merged
merged 2 commits into from
Feb 11, 2025

Conversation

dummdidumm
Copy link
Member

  • $derived can contain $state declarations so we cannot ignore them, so this reverts fix: don't try to add owners to non-$state class fields #14533
  • instead, we add equality checks to not do this expensive work unnecessarily
  • this also adds a render effect similar to the class ownership addition when it detects a getter on a POJO during ownership addition

fixes #15164

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
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

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

Copy link

changeset-bot bot commented Jan 31, 2025

🦋 Changeset detected

Latest commit: 67dc579

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

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@svelte-docs-bot
Copy link

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15166

- `$derived` can contain `$state` declarations so we cannot ignore them, so this reverts #14533
- instead, we add equality checks to not do this expensive work unnecessarily
- this also adds a render effect similar to the class ownership addition when it detects a getter on a POJO during ownership addition

fixes #15164
@dummdidumm dummdidumm force-pushed the ownership-derived-fix branch from e35f074 to f0bb9c5 Compare January 31, 2025 15:08
@henrykrinkle01
Copy link

Thanks for the quick fix. Could you have a look at this case as well?
https://svelte.dev/playground/de8dcaf1a1ef4b588dde10c825212c11?version=pr-15166

@dummdidumm
Copy link
Member Author

Mhm, honestly I think this is a case which we cannot solve. But also, if you're passing along a thunk, you may also pass an update function.

@trueadm
Copy link
Contributor

trueadm commented Feb 11, 2025

Did this PR go stale?

@dummdidumm dummdidumm merged commit a3e49b6 into main Feb 11, 2025
9 of 10 checks passed
@dummdidumm dummdidumm deleted the ownership-derived-fix branch February 11, 2025 22:07
@dummdidumm
Copy link
Member Author

No - I just didn't see Rich approved it; thanks for the reminder 😄

@github-actions github-actions bot mentioned this pull request Feb 11, 2025
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.

Object ownership not added for $state inside $derived
4 participants