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

New decorators and enumerability #4248

Open
fforw opened this issue Feb 1, 2025 · 8 comments
Open

New decorators and enumerability #4248

fforw opened this issue Feb 1, 2025 · 8 comments
Labels

Comments

@fforw
Copy link

fforw commented Feb 1, 2025

I'm just catching up on MobX and the new decorators with the accessor syntax, which I don't really like (far more noise than one makeObservable call), but I get it.

What I don't get is the fact that the new properties are not enumerable, and not own properties?

If we have a very basic class like

class Foo
{
    @observable accessor name = "Unnamed"
     num = 1
}

in what world does it make sense that "name" is neither an own property nor enumarable but num is?

This makes toJS() totally useless, especially for stuff like just logging and knowing what the hell is going on in my application. Same for JSON.stringify(). Useless.

So now I need to write manual JSON-ification schemes or use external libraries just to know what is going on in my application?

The reasons given in https://mobx.js.org/enabling-decorators.html / "Decorator changes / gotchas" seem incredibly hand-wavy compared to these actual problems?

"following the spirit of the ES language and what accessor means" is nice, but how about you follow what MobX means and what it presents as abstraction to the user? Same for OOP purity. Is there any other situation where MobX enforces OOP purity at the expense of functionality?

@fforw fforw added the 🐛 bug label Feb 1, 2025
@mweststrate
Copy link
Member

If you want to know a bit more about the reason why it works the way it does, would you mind rephrasing the question in such a way that it touches more on the factual matter of your problem and less on your value judgement about it?

@fforw
Copy link
Author

fforw commented Feb 2, 2025

The "value judgements" result in very concrete problems.

The facts are that his behavior conflicts what normal Javascript objects are. Unless we go through the different kinds of pains of hiding things, every property is enumerable and is owned by the containing object. Why should that be different from observable properties?

toJS / JSON.stringify are just broken as are any way of treating OOP objects in a generic way. Functionality just gone. No more easy logging, we need an elaborate JSONification scheme for our domain. And the only reasons we get for that are your value judgements about closeness to the ECMAscript decorators standard and OOP purity.

I'm unhappy with losing all that in the code that uses MobX.

@mweststrate
Copy link
Member

Let's stick to the concrete problems then. Telling me what my convictions are is at best very presumptions.

Fwiw I'd agree in general it'd be great if these would be own and enumerable, it's a while ago since I worked on it, but I recall the spec didn't allow for an accessor property to be installed efficiently as own property on a targeted instance (for accessors the target is prototype), or it wasn't possible to compose them .

But happy to be proven wrong if you have a small sandbox or something that does set these properties correctly for a noop accessor decorator.

@Braidner
Copy link

Braidner commented Feb 8, 2025

I’m not sure if this is the right place to mention this, but I wanted to add that with the new decorators (2023-11), the toJS function no longer works as expected. The returned object is always empty. Additionally, apiOwnKeys only returns a single Symbol(mobx administration) instead of the expected keys.
sandbox

@jzhan-canva
Copy link
Contributor

jzhan-canva commented Feb 11, 2025

accessor is essentially a get and set on prototype

class Foo {
  bar = "bar"
}
const foo = new Foo()
JSON.stringify(foo) // prints '{"bar":"bar"}', because `bar` is in `foo`
class Foo {
  get bar() { return "bar" }
}
JSON.stringify(new Foo()) // prints '{}', because `bar` is in `Foo.prototype`

FWIW, getter in object is enumerable

const foo = {
  get bar() {
    return "bar";
  }
};
JSON.stringify(foo) // prints '{"bar":"bar"}'

@jzhan-canva
Copy link
Contributor

jzhan-canva commented Feb 11, 2025

I agree this is a big breaking change, losing enumerability may cause many APIs to work unexpectedly after the migration to modern decorator

@jzhan-canva
Copy link
Contributor

I suggest we add a warning to new decorator doc before we come up with a solution

@mweststrate mweststrate mentioned this issue Jun 6, 2024
18 tasks
@mweststrate
Copy link
Member

I just realize (mostly as a stop-gap) that toJS itself probably should be updated, e.g. instead of the apiOwnKeys we should probalby just lookup the observable adminstration and map over the internal properties (filtered on ObservableValues): https://github.com/mobxjs/mobx/blob/main/packages/mobx/src/types/observableobject.ts#L104

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

No branches or pull requests

4 participants