PoC annotations + subclassing rework #2641
Conversation
🦋 Changeset detectedLatest commit: 8cb03b5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
@urugator thanks for the awesome work! For the class approach, I want to check if I understand everything correctly, so here a summary in my own examples: observableon instance, getter/setter, non configurable. So inheritance works like: class A {
x = 1
constructor() {
makeObservable(this, { x: observable })
}
}
class B extends A {
x = 2 // FAIL
constructor() {
this.x = 2 // CORRECT
}
}action on methodon proto, configurable, non writeable class A {
method() {
}
constructor() {
makeObservable(this, { method: action })
}
}
class B extends A {
method() {
}
constructor() {
// works, as it wraps B.method, not A.method
makeObservable(this, { method: action })
}
}action.bound on methodon instance, non configurable, non-writable class A {
method() {
}
constructor() {
makeObservable(this, { method: action.bound })
}
}
class B extends A {
method() {
}
constructor() {
// NOT SUPPORTED
}
}
// Aternative pattern
class A {
method() {
}
constructor() {
this.method = action(this.method.bind(this))
}
// OR
method = action(() => { } )
}
class B extends A {
method() {
}
constructor() {
this.method = action(this.method.bind(this))
}
}action fieldon instance, non-configurable, non writeable class A {
method =() => { }
constructor() {
makeObservable(this, { method: action })
}
}
class B extends A {
method = () => { } // FAIL, see above work around
constructor() { }
}computedon instance, getter/setter, non configurable class A {
get x() { return 1 }
constructor() {
makeObservable(this, { x: computed })
}
}
class B extends A {
get x() { return 2 } // FAIL
}
// work around
class A {
get x() { return this.getXHelper() }
getXHelper() {
return 1
}
constructor() {
makeObservable(this, { x: computed })
}
}
class B extends A {
getXHelper() {
return 2
}
}I think it deviates from the current PR in the sense that action on the prototype are configurable (could even be writeable?). The reason is that non-configurability doesn't make a practical difference from our perspective(?) (since the subclass implementation lives on a different prototype), but this approach does allow for stubbing still. I'm not sure if I see the value of Other question, not sure if this is a patch, or a major release. It is a pretty breaking change, on the other hand, it mostly fixing things that didn't work correctly before, or accidentally, and brings some things back in line with MobX 5, like enumerability of methods, which was a non intentional change of 6 IIRC? |
Correct
Correct, but you must not re-annotate in B - it will throw ... you can optionally use
Overriding class A {
method() {
}
constructor() {
super()
makeObservable(this, { method: action.bound })
}
}
class B extends A {
method() {
}
constructor() {
makeObservable(this, { method: "override" }) // optionall
}
}
Correct
Correct
I use the same rules for prototypes, more or less to keep the code simple. I mean modifying a prototype is problematic in the same way, so the non-configurable/non-writable does make sense. Whether users actually ever attempt to modify prototype, I dunno, probably not, therefore this safety measure may not be strictly required.
If I allow applying
Only if the parent is abstract.
I don't know either :) I tried to come up with something that makes sense as whole and fits together. I may went too far, but I don't think there are many possible solutions. BC 1 - non-configurable/non-writable:It's just a safety measure, but unfortunatelly quite important in the context of the solution: We tell the user: "You can't re-annotate, the annotation is inherited implicitly" - except it's no longer true, if he changes the field definition in subclass... BC 2 - re-annotating is forbiddenThat's something the solution relies on. The docs have to be changed and users must adapt the code. BC 3 - EnumerabilityCurrently (in main branch) all actions are non-enumerable, same as in previous versions. |
|
Few more Q&A to clarify reasoning/decisions:
No, because we don't know which annotation belongs to which prototype
In theory yes, when new annotation is provided we would completely destroy and recreated the field. User must provide new field definition/value - he can't change the annotation only (no easy way to ensure this).
It doesn't make sense to change the field type and therefore it's annotation+configuration neither in subclass or over time.
No, we would have to keep a copy of all descriptors and compare them with current descriptors in every
No,
No, because we don't know which prototype belongs to which
In theory yes, problem is we don't know which annotation was applied to which field. Since the field was redefined, we can't even use some reflection to find out. So we would have to keep a list of applied annotations on instance. |
|
| decorator | target | with inheritance |
|---|---|---|
| observable | field | cannot be redeclared, to avoid (accidental) shadowing we mark it non-configurable |
| action | method | can be redefined in subclass, but not redecorated, except with override |
| action.bound | method | same |
| computed | getter / setter | same as action (so can be subclassed, but not re-decorated except for override) |
| action / action.bound | field | own field, but non-configurable / writable to avoid accidental shadowing, work around: manual action wrapping |
Enumerability
For background, the reason why getters are made to be non-enumerable, is that they aren't enumerable if part of a class definition either. The change between MobX 5 and 6 was not so much that actions have become enumerable, but more that the default decorator for methods in an object literal was observable, rather than action. So the reasoning so far was that observable({ field, method, get computed }) behaves practically the same (except for ownership) than new class { @obervable field @action method @computed get computed }). So I think it is kinda of a binary choice now where we have to determine the philosophy, we make observable either behave like as close to a cloning function as possible, or as close to creating a class instance as possible, where the first arg is basically a model for the new instance.
I think personally I lean towards the second philosophy, which I think works very well in practice (not a lot of issues reported on it so far), but I could live with the first one as well. In that case we should address the enumerability of computed indeed as well; they should be enumerable if defined on an object, but non-enumerable if defined on a class playgroud
Not sure it it's meant to be a summary of how you understand the PR or proposal for changes:
In current PR: Re-annotation is simply never allowed for anything, except for
I was convinced that
As already discussed on gitter, that's one thing I would like to change as well (not part of the PR atm). I want to revert this to use
Imo it's not so binary, because in one case (close to class instance) we take away the choice from user. If we respect the descriptor, the user can change it however he wants before passing the object to our non-opinionated/non-surprising function. |
Sorry, forgot to update that line while reading. Will edit. But yes, my summary ought to be: 'can be redefined in subclass, but not redecorated, except with
I thought initially it to be tricky as well, but than I figured it probably isn't: in the first
In practice most people don't know how to do that, and even if they do, it is terribly verbose to write, using uncommon api's, so I think in practice people will then correct for it in another place (for example during json.stringify / iteration). I'd rather have the opposite: by default stringify / iteration does sane things, and if you really want to see those members, update the iteration code (e.g. using getOwnPropertyNames or something alike). Having this preconfigured by default also more closely aligns with the distinction we make in the docs: there is state, and separately there are methods and derivations that operate on the state. But in reflection cases, you are typically mostly interested in the state. I think this also addresses the other problems: not making all methods observable (which is expensive by default, and a bit suprising as well, computeds aren't reassignable members either). In the case where you do care about logic changing over time, for example a filter predicate that is being stored, marking it as I think it is a pretty clear model, even if it deviates from how plain objects work. But these are not plain objects, these are objects within a framework that has a lot of concepts that javascript doesn't have, like the explicit distinction between state -> actions -> derivations, and I think the current behavior reflects this philosophy behind the library. The more I think about it, the more I'm convinced we just need to make sure it is properly documented. In either solution we'll get surprises, but I rather have the suprise when people deviate from the mental model. If a suprise occurs: 'hey, these methods aren't enumerable', the follow up question should be: 'why do I need those to be enuemrable'? So between 'stringify doesn't work, but object spreading does. But hey, don't do that, it is probably not what you want!', and 'stringify is pretty neat ootb. Ok, spreading won't work, but you shouldn't do that anyway', I think the latter is the better choice :) |
|
The idea was that they can easily introduce own
Note we are not making observable something that wasn't observable before. Previously these functions were observables, now they would be additionally batched - so now they would be actually usable as methods (they wasn't before) Things explicitely annotated by "However, I think we may reconsider extendObservable behavior as well. It doesn't makes sense to make non-observable fields (action/flow) writable - if the field is writable, then it's statefull and therefore should be observable, otherwise we are risking staleness. What I don't like about it is it simply deviates, you need a rule for that and it can get weird: // it doesn't have to be in class
this.field = action(() => {}); // enumerable
// but
this.field = () => {}
makeObservable(this, { field: action }) // non-enumerableI just don't feel like that this implicit opinionated "convinence" we would provide (and again user can create a function that does this for him) is worth of complicating the impl and the rules. |
Disagree, a utility that makes something non-enumerable is much harder (you can't create a util: JavaScript itself picks already different defaults in different scenarios as well, classes make getters and methods non-enumerable by default for instances, in contrast to plain objects, even though you didn't specify that anywhere, so it isn't that weird. Following the same consistency reasoning we shouldn't make anything non-configurable, as that deviates definitely from what the language does ootb. We do it nonetheless in this PR, because it provides a better DX in the end. I think the same holds for enumerability. Converting enumerability is just as arbitrary as converting configurability, or fields to getter/setter pairs. These are all just as inconsistent. I would care for consistency if it was like a 30% (enumerability would be great in this case) / 70% case (non-enumerability is more convenient here). However, I still don't see any good use case yet justifying why we want actions to be enumerable from a practical perspective, and as long as that isn't the case, I don't see how consistency with plain JS objects even matters. And personally, I'd argue non-enumerability is actually more consistent: regardless whether you use object or class style MobX, you get pretty similar instances in the end. It is less consistent on the axis of objects in JS, but more consistent on the axis of making objects observable with MobX. So tl,dr, I think we should move ahead with the inheritance changes in this PR including |
|
Should I make Fields in What about that testing/stubbing? Btw, it's probably not of any use, but I've just noticed one can do: class A {
o = 5;
constructor() {
return new Proxy(this, {
defineProperty() {
throw new Error('Cannot redefine')
}
});
}
}
class B extends A {
o = 10;
}
new B(); |
Sounds good!
Correct. I think we can assume the same for
For |
|
We could also make fields non-configurable only when annotating non-plain object. That way |
|
I am currently thinking about how to reconcile makeObservable/extendObservable and dynamic objects. const o = observable({ foo: 1 as any })
o.foo = () => {}
makeObservable(o, {
foo: action
})
isAction(o.foo) // false
isObservableProp(o, "foo") // trueI would like to solve this by preventing re-annotating in general, so the only way to re-annotate would be to delete and reintroduce the prop. Ideally I would like to have this check on a single place. So basically I need to move |
|
Yes feel free to limit these kind possibilities, and think so far the behavior of such constructions has been undefined. For enumerability feel free to do what you feel is best for object literals. Idealistically speaking I like non-enumerability, but I see that people might expect enumerability, especially since that was (kinda unintentionally) what we did before. So one could consider that that ship sailed a long time ago already. So I think either approach is defendable. |
Should
|
No, as the docs suggested, this isn't necessarily observable. I think it was in the past primarily by coincidence as the
No strong opinion on this one so far. The discussion behind this one is probably a bit similar to the one behind enumerability; the primary use case for having observable object extension (I actually don't know another one), is using an object as a collection (in other words a Map) of other observables. For that reason I don't think there is much value in having notifs about new computeds / actions (one could even wonder if an object is used as collection, whether it should have computeds / actions at all). However, if it simplifies things, without slowing them down much, I think it could be ok. But what would worry me is what would this mean for things like intercept. Does it mean that adding an action can be intercepted as well? That might make things fairly complicated. |
That's the main goal. Currently the whole thing is quite quirky ...
Well the plan was to really interecept everything, because I assumed that everything goes through Many of these situations may be unlikely to happen or of any use, but I want to tackle them somehow as they make the code hard to reason about. EDIT: |
|
@urugator Supporting non observables in |
|
Hi,I am coming from an error thrown when extending classes with Sorry if I didn't follow all the messages in this thread, but my question is simple: will makeAutoObservable work with super() / subclasses after all these changes? Context: I have a couple of projects where there is a "BaseStore" class that is used everywhere. It encapsulates a couple of extras available to any store, to keep the code DRY, and many of my stores work just fine with makeAutoObservable. So this pattern would be used a lot without the error: |
Atm it will throw as well. But the way it's implemented in the PR, I think it could actually work as long as you call |
|
I understand thanks. Except for rare cases, all my stores are configured in the same way. I only use custom config if there is some issue with makeAutoObservable. |
|
could makeAutoObservable add a symbol / hidden property to its target then check for that property before applying itself, then if the hidden prop exists, just do nothing? basically like: class Dog extends Animal {
constructor() {
super()
if(!this.prototype.__@@autoobservable__) makeAutoObservable(this)
}
} |
|
No, because it would ignore fields defined by subclass. However there are other problems. Eg we always annotate whole proto chain (because we don't know which proto belongs to which call), so if you extend 3rd party class like |
|
Bah. Maybe the first makeAutoObservable call could create an undecorated copy of the class and store it either on itself or in a external map so subclasses could reference it? kind of like a "class Dog extends preconstructed Animal" equivalent. class Dog extends Animal {
constructor() {
super()
if(this.prototype.__@@mobxcopy__) { //<- copy of Animal before makeAutoObservable ran
// delete things if needed here
Object.assign(this,
makeAutoObservable(Object.assign(this, this.prototype.__@@mobxcopy__)
)
}
}
} |
The problem
We can't tell which prototype belongs to which
makeObservablecall, therefore:In case of annotation, we don't know which annotations apply to which prototype.
In case of decorators, we don't know which decorators belong to which
makeObservablecall.The solution
Make it so that it doesn't matter where and which annotation is applied. In order to do that we need 2 guarantees:
Annotation and it's configuration cannot change in subclass - subclass can't switch
actiontoflow, but alsoaction("1")toaction("2")etc.One way to handle this would be to compare newly provided annotation with the one already applied, however that's generally not possible, because annotation are not easily comparable, eg:
@computed({ equals: (a,b) => a === b })So another way, the one used in this PR, is to make it impossible to annotate the same field twice.
This means that when you override something in subclass, you must not annotate/decorate it again - the annotation is sort of automatically inherited. Actually you don't even need to call
makeObservableagain unless you add new fields.The requirement to leave some fields un-annotated could lead to actually forgetting an annotation. Therefore I introduced a new annotation/decorator
override, that denotes the intention and checks if the field is truly annotated in superclass.The field definition cannot change.
Redefining the field in subclass would mean that we have to re-apply the annotation. However detecting such situation is not possible. At the same time the user cannot re-annotate re-defined field explicitely, because re-annotating the same field multiple times is forbidden.
Therefore we forbid redefining the field by making it non-configurable and additionally non-writable for action/flow.
As a consequence these are not possible:
Alternative solution with writable actions (but still not configurable)
Additional changes/notes/controversies:
Most of the safety checks are done on devel only.
Everything that's not supported should throw (let me know if I missed something).
It's not always possible to provide meaningful/instructive error, but I think it's better than breaking app silently.
Enumerability of all members is respected (BC), therefore:
[this.]action = () => {}is by default enumerableThis change is strictly not necessary, but I aim for: #2586 (comment)
See also #2629, #2637
Same goes for
observable()/extendObservable()- even explicitactionwon't make the field non-enumerable, if it's enumerable on source object.We could do this only for autoconverted members, but I think it would be nice to have the same behavior across the board.
extendObservableandmakeObservablehas now slightly different semantics:makeObservableis intended mainly for classes and assumes static object shape (non-configurable fields/non-writable methods). Stubbing is therefore impossible (at least on devel) 🙁extendObservableis usable with dynamic objects - field can be deleted, methods can be rewrittenHowever, I think we may reconsider
extendObservablebehavior as well. It doesn't makes sense to make non-observable fields (action/flow) writable - if the field is writable, then it's statefull and therefore should be observable, otherwise we are risking staleness.So either make everything observable, or make non-observable non-writable.
If you extend 3rd party class and annotate inherited method, it will annotate the 3rd party class as well (eg turns it's method into an action if it's annotated as such in subclass)
Overriding computed is not supported as before, but now it throws.
Fixed/renamed/commented a few things here and there.