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

Svelte 5: $state rune on classes breaks enumeration #9699

Closed
JanNitschke opened this issue Nov 29, 2023 · 7 comments
Closed

Svelte 5: $state rune on classes breaks enumeration #9699

JanNitschke opened this issue Nov 29, 2023 · 7 comments
Labels
awaiting submitter needs a reproduction, or clarification

Comments

@JanNitschke
Copy link

JanNitschke commented Nov 29, 2023

Describe the bug

using the $state rune on class properties hides them from Object enumeration.
Object.keys(), for in, spreading to clone do not work as I would expect.

<script>
class ToDo{
  id = 0
  name = $state(name)
  constructor(name){ 
    this.name = name;
  }
}
Objects.keys(new ToDo("test")) // ["id"]
for(let prop in new ToDo("test")){
  console.log(prop) // "id"
} 
console.log({...(new ToDo("test"))}.name) // undefined
</script>

Cause

The compiler changes the name field on the instance to private (#name) removing it from enumeration. The getters and setters are created attached to the class/prototype instead of the instance. The way they are set also hides them from direct enumeration on the prototype.
The behaviour of enumeration in JS in combination with prototype chains leads to the rune having a bigger effect than most developers would expect.

This could be fixed by injecting Object.defineProperties(...) into the constructor instead of getters and setters on the class.

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAE21TyY7bMAz9FVadg40JnJ6zDFB0-YLe4hw0Eu0IVSRDotMWhv-9oqxkkk4vlri8x0eRnkRnLEaxOUzCyTOKjfg8DGIl6M_ARrygJUx29GNQ7NlFFcxAL61rySJBbzTs4dOWbWVljPDDf_UTmy3lWMp4ft4uDq6RXE-RJGHVilbUJaK8ixRGRT5UnFUXipboZGJTgHwUwMwHf4oQ8trHlHJw-CtrSPTj0AepEZY2UrEV3EW_m9_wOvYx-eG4EGUVQEGqn19O0vXIjBVaPKOjGvYvUGRxRW-5valpmpIwF2lvHXmLjfV9lVJXULJWDMwN1VtYr3NPYCI4TzAE4wg1SKezrax3qO9Zn7DrUFFV3atpqfMBKhY1BD-AcXDTfEu5MvyrjRE1C9EeFxWGMKT5gL9gyPL-R2G667scmOEIH_Z77myx6vuyD-VaofLL6las4FbbIddS0tpbu9cZP1zev3nZhnrRls28Ck16km9SnarqYXD3s02RPLEE5pjbrd-W200fMaHLWsmYL5l9Z9wwErwapzcXaUfcTxzLE51hncDTmqFz-m3OXpvOoBabtNs4H-e_3BKzFnEDAAA=

Logs

No response

System Info

Tested in Chrome and Safari on macOS

Severity

blocking an upgrade

@JanNitschke JanNitschke changed the title Svelte 5: $state rune on classes breaks iteration Svelte 5: $state rune on classes breaks enumeration Nov 29, 2023
@brunnerh
Copy link
Member

brunnerh commented Nov 29, 2023

Runes in classes get transformed into get/set properties which live on the class prototype, the observed behavior is just a result of that as that applies to all classes.

In the final release it probably should be documented that these side effects are to be expected.

(You should not try to spread class instances anyway, you lose the prototype.)

@JanNitschke
Copy link
Author

@brunnerh The transformation of runes into getters/setters on the prototype makes it extremely difficult to write generalised code using classes that include runes. Adding getters and setters to a class does not mark the property as enumerable on the class as well as the object. It does not only break spreading, but Object.keys, Object.entries, Object.values and even the in operator and for in loop.
I noticed that I could not write a function to track changes on a generalised object without looking into the property descriptors of its prototype.

Since it changes the behaviour greatly and there is no trivial workaround, I don't think this problem just needs to be documented.
A solution could be to attach the getter/setter to the instance. This would make the behaviour identical to properties without the rune.

@brunnerh
Copy link
Member

A solution could be to attach the getter/setter to the instance.

That is not really an option. The whole point of the classes approach is to provide raw performance and this would put a large dent in that.

Runes are still in flux, in the end you might not need to use class syntax unless for very specific cases where the performance is more important than the ergonomics.

@JanNitschke
Copy link
Author

JanNitschke commented Nov 29, 2023

Runes are still in flux, in the end you might not need to use class syntax unless for very specific cases where the performance is more important than the ergonomics.

Making runes directly usable in objects would be a great solution. If this is not possible the compiler could also mark the properties as enumerable on the class prototype. This would at least allow for in loops to iterate over the object.

class Test{
  get prop1(){ ...} 
  set prop1(){ ...} 
  get prop2(){ ...} 
  set prop2(){ ...} 
}
Object.defineProperties(Test.prototype, {
  "prop1": {
    enumerable: true
  },
  "prop2": {
    enumerable: true
  }  
})

While this is a pretty ugly solution, I don't think this would have any performance implications except making the bundle slightly larger.

@trueadm
Copy link
Contributor

trueadm commented Nov 30, 2023

@JanNitschke Unfortunately, due to how we implement this on classes (we use private fields) the Object.defineProperties would need to be within the class body – likely the constructor. I measured, and doing it from within there adds quite a lot of code bloat and slows down the construction of class instances quite a bit – even if we avoid doing it again on further calls.

Personally, I'm of the opinion that maybe this is best solved using extends, i.e.

class Text extends Record { }

Where Record or some class basically provides utility functions that provide helper methods like toJSON, toObject, keys, values and entries. It could even provide next and Symbol.Iterator and make it so you can for…of the instances which would be nice. However, that's not something we think needs to be done right now. We have some other ideas to make state more approachable and so we'll first do that, with the expectation that classes won't be as heavily used in patterns where you might have to enumerate the keys.

@dummdidumm
Copy link
Member

dummdidumm commented Mar 7, 2024

What's the use case behind this that can't be solved with a POJO (i.e. let object = $state({ .. }))?

@dummdidumm dummdidumm added the awaiting submitter needs a reproduction, or clarification label Mar 7, 2024
@Rich-Harris
Copy link
Member

Closing this, as I suspect $state({...}) is the answer here — this issue predates #9739, and so that wasn't an option at the time. The cases where it's appropriate to enumerate the properties of a class instance are very rare, and as noted above it would defeat the object to put the getters on the instance rather than the prototype.

@Rich-Harris Rich-Harris closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting submitter needs a reproduction, or clarification
Projects
None yet
Development

No branches or pull requests

5 participants