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

Regression: Silence ownership warning for object class fields #11060

Closed
michael opened this issue Apr 4, 2024 · 4 comments · Fixed by #11094 or #11184
Closed

Regression: Silence ownership warning for object class fields #11060

michael opened this issue Apr 4, 2024 · 4 comments · Fixed by #11094 or #11184
Assignees
Milestone

Comments

@michael
Copy link

michael commented Apr 4, 2024

Describe the bug

According to Dominic "Classes are excluded from the warning as they're deemed "you know what you're doing."

While everything works fine with flat class fields (like strings) we do see the warning for class fields that hold an object.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE51Uy66bMBD9lalVCSIR2JOHVLXdVKrURXelqhwYgnOJjewhuRHyv1fGQOBGV0q7Mp7xnHmcM3SsFDUalv7qmORnZCn71DQsYnRr3MVcsCZkETOq1bmzbE2uRUP7TGYkzo3SBD9QGyW_FoKUhlKrM2QsTubW2ONkbPMQ9l0VWL-N6o1DUHwyy7gODNJnJQlfKYLj9A12hFlkq5Gg6VFhBx8NccJQ4nWePuxc7yl0pdCG_vhL8E1VMoig5nfTF4WBjaBSh8MtheAnSilMYFcrl0nCrLAwY7wRGYugcw5wdQ5lhKvRBqCRWi0Hx8ZbbeRO6zC3yX3acruYc7LP5LYQl56IzgPErsz43oSFhWPqxEItXtBM3r4fG7t8DpFF7KwKUQosWEq6RRtN6pjX8I8y-Q_eeCNgN3s6TvVxNh_W65FlYYBLENIQlzmCKhdKo4oTVNx4EiE0pIU8roDLAlyPEKrDCXNaQV5zY6AUWBcmhvV6P6Q5CFkIeQRSLo9_PX8Mtz4EOKirRG0q0QBqrfSAIWTTUo-SXnjd4q7jjYjfYzB5LmTGbTIVOsSMxYLvdVHrVekXA1dBlWoJOFy5lu7NE6V61UDyhF7m-3xXzcnMFYOvvUoKLHlbjwOdM9c5VfQUTXvcwWJj32xr0O9QRp7pKSgI_LpmlCtpSLc5Ke1_AcNq25VPlhFVwsRDTndsZuYRtj97h82kfRzGb_sX8jyRCmYFAAA=

Logs

"PersonEditor.svelte mutated a value owned by App.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead."

eval (playground:output:2748:39)
untrack (playground:output:1700:11)
console.<computed> [as trace] (playground:output:2748:16)
check_ownership (playground:output:603:12)
Object.set (playground:output:889:6)
eval (playground:output:2626:117)
HTMLInputElement.eval [as __on_r] (playground:output:2414:4)

System Info

System:
    OS: macOS 14.2.1
    CPU: (8) arm64 Apple M2
    Memory: 90.53 MB / 8.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.10.0 - /usr/local/bin/node
    npm: 10.2.3 - /usr/local/bin/npm
  Browsers:
    Chrome: 123.0.6312.105
    Safari: 17.2.1
  npmPackages:
    svelte: 5.0.0-next.93 => 5.0.0-next.93

Severity

annoyance

@dummdidumm
Copy link
Member

I don't think classes should be exempted from this, but it's still wrong that the error shows up here because it's passed into context at which point the ownership should be widened.

@dummdidumm dummdidumm added this to the 5.0 milestone Apr 4, 2024
@paoloricciuti
Copy link
Member

I don't think classes should be exempted from this, but it's still wrong that the error shows up here because it's passed into context at which point the ownership should be widened.

if that's true this should also apply to object literal right?

@michael
Copy link
Author

michael commented Apr 4, 2024

I don't think classes should be exempted from this, but it's still wrong that the error shows up here because it's passed into context at which point the ownership should be widened.

Interesting. Then ownership should also be widened for state (deep object) owned by a component like here?

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE51RwWrDMAz9FSEGbcEkx0KaFsa2--7zDl6jdi6JbWylrBj_-3DadCkbg-1k-0nvSe854k63FLB6iWhUR1jhvXMokE8uP8KRWiYUGGzvtxmpw9ZrxxtpJOvOWc_wTD5Y89Roth523nYgsSinaHHWkbia0CIE4gdrmD5YwP56hzSK3LBaYnCDJqzhLrBimse8cwWzg3038GhpJkDtqYLlMi0yycBkxlyiclqigJgLkEdeFOeLEQPwxL03l8LqjCaRz0GzLr8CMPWN9XIjTd3o45BNPAsUecMEOsAIqD0lOJHyAWzbFFkxc1BgZxu909Rgxb6nJK5fMp3yx7_5R8jKaVhPWsfcvrvXxvUMb9o01VG1Pa2jcrq4MT5k8mvfkEf5g__X9An-wXzkngIAAA==

Here's the original Twitter thread for context: https://twitter.com/trueadm/status/1775864362953048451

@dummdidumm dummdidumm self-assigned this Apr 8, 2024
dummdidumm added a commit that referenced this issue Apr 8, 2024
Previously, ownership widening/removal was only done if the immediate object that was encountered was state. This isn't always the case. It also didn't take into account classes with state on it (which turn into getters). This change takes both these cases into account and now always traverses the given object deeply.
fixes #11060
dummdidumm added a commit that referenced this issue Apr 8, 2024
Previously, ownership widening/removal was only done if the immediate object that was encountered was state. This isn't always the case. It also didn't take into account classes with state on it (which turn into getters). This change takes both these cases into account and now always traverses the given object deeply.
fixes #11060
@Rich-Harris
Copy link
Member

reopening since #11094 was reverted

@Rich-Harris Rich-Harris reopened this Apr 13, 2024
Rich-Harris added a commit that referenced this issue Apr 15, 2024
Rich-Harris added a commit that referenced this issue Apr 16, 2024
* rename metadata.o to metadata.owners, tweak check_ownership implementation

* track parent relationships

* update

* changeset

* adjust test to reflect new semantics

* prevent component using bind: with object it does not own

* failing test

* fix #11060

* add explanatory comment

* don't accidentally narrow global ownership, fix has_owner method

* widen ownership if assigning different state proxies to one another

* don't set owners to null when parent exists

* fix

* only recurse into POJOs

* handle cycles on context

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
Co-authored-by: Dominic Gannaway <dg@domgan.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
4 participants