Closed Obi-Dann closed 2 weeks ago
You still have to call makeObservable(this)
in constructor:
https://mobx.js.org/migrating-from-4-or-5.html#upgrading-classes-to-use-makeobservable
There is a tool to help you with migration: https://mobx.js.org/migrating-from-4-or-5.html#upgrading-your-code-with-the-mobx-undecorate-codemod
And also eslint rule https://github.com/mobxjs/mobx/blob/main/packages/eslint-plugin-mobx/README.md#mobxmissing-make-observable
@urugator not according to the docs when using modern decorators.
All other decorators work find without makeObservable(this)
when using modern decorators
https://mobx.js.org/enabling-decorators.html#enabling-decorators
I see. I am not completely up to date with these, just slapped the makeObservable in the stackblitz and seemed to work.
There was a discussion about this, dunno what the resolution was: https://github.com/mobxjs/mobx/pull/3638#issuecomment-1481858285
Thanks for the link, I fully agree with this comment https://github.com/mobxjs/mobx/pull/3638#issuecomment-1482420519
I think this should be fully working as intended, or fully broken at least on type level.
In the current state, it's just confusing. If @action
on a field is not supported, it should be clearly mentioned in the docs and the method should not accept ClassFieldDecoratorContext
so it fails at the compile time.
If the goal is to only make it available with makeObservable
, then again, the docs should not state that makeObservable
is no longer needed.
I find the decision of not supporting @action
on fields adds unnecessary friction, confusing, not communicated and unsound in it's current state.
Is it possible to just support the @action
on the fields, it's essentially one line change as @Amareis mention:
return (mthd) => _createAction(mthd)
As for the limitations and potential issues on reassigning the value on @action fields, as @Amareis also mentioned, it's the same as reassigning @action methods https://github.com/mobxjs/mobx/pull/3638#issuecomment-1483996867
cc @Matchlighter @Amareis @mweststrate
@Obi-Dann I've been out of the MobX code base for a bit, but from a JS stand point, field decorators cannot augment the value of the field. They're annotation only. It's not the same as re-assigning methods. So JS-wise the only way to achieve @action
on fields was to make them accessor
s or to use makeObservable
.
Now, with newer changes to the decorator spec (addInitializer()
entries now run in order with field init - not before field init), MobX might now be able to use addInitializer
to apply the @action
, but this would have some nuances compared to the original (MobX 4 era) behavior.
It'd be up to @mweststrate (or anyone more current in MobX) to make this call. I know our original thought was to lean more in to the modern JS design and (going forward) limit @action
to methods - with use on fields (via makeObservable()
) considered as deprecated as makeObservable()
. (I have a hard time seeing the desire to use a field over a method with @action.bound
, but I always get mad when library devs don't give me escape hatches, sooo...!). Obviously my approach here has led to some frustration though.
I agree that the docs I wrote probably didn't state this as sharply as they could have.
@Matchlighter If I read the spec correctly, field decorators can augment the value of the field https://github.com/tc39/proposal-decorators?tab=readme-ov-file#class-fields
Not sure it's always been the case, but it's definitely possible now and actually works
In terms of the behavior, the logic converting a function into an action would run once during the initialization. The logic would not run if the value is reassigned.
It's pretty much the same as using @action
on a method
class Store {
@action
x1 = () => {};
@action
x2() {}
}
const store = new Store();
console.log('x1 isAction', isAction(store.x1)); // true (assuming this issue is fixed)
console.log('x2 isAction', isAction(store.x2)); // true
store.x1 = () => {};
store.x2 = () => {};
console.log('x1 isAction', isAction(store.x1)); // false
console.log('x2 isAction', isAction(store.x2)); // false
Found an additional discrepancy when running the code above, calling makeObservable(this)
on that store makes x1
readonly, but I can still re-assign x2
at runtime without any issues.
In terms of the using the field vs @action.bound
.
There's useful eslint rule https://typescript-eslint.io/rules/unbound-method that reports false positives for methods with @action.bound
.
Let me revisit this when I find a bigger chunk of time. Also in combination with #3882. I have a vague mental note that at the time of writing there was an issue in the transpiler with the addInitializers.
@mweststrate I've got a PR with the fix for this issue https://github.com/mobxjs/mobx/pull/3883
Not sure this issue is related to https://github.com/mobxjs/mobx/issues/3882. There are currently tests for both action methods and action fields, it looks like it's intended behavior but only action methods were supported to run properly without makeObservable
Hi @Obi-Dann I just created another PR #3902 based on yours #3883 although it's very rare but I think we need to handle assigning traditional function to property
class A {
@action.bound
doSomething = function() {...}
}
Hi @mweststrate, any chance we can get this issue solved? I tested PR #3883 on the project I work and it works perfectly. I also created PR #3902 to handle one extra edge case
Hi @mweststrate, just wanted to kindly remind you to review PR #3902 whenever you get a chance. Thanks!
Do we have tests for inheritance with modern decorators? If the behavior differs in any way from legacy decorators/makeObservable, I would like to see it documented. Eg. legacy decorator/makeObservable don't allow re-annotating field in subclass and they implicitly annotate whole prototype chain. I don't want to go over ever single case here, I just want to have an affirmation that someone gave it a thought. https://mobx.js.org/subclassing.html
@urugator If there were tests for such for legacy decorators - yes. I pretty much just duplicated the whole decorators test file and ported it to use 2022.3 decorators.
Hi @urugator please find updated doc in my PR also added a new unit test to demonstrate the behaviour
@Matchlighter We do, but they are here: https://github.com/mobxjs/mobx/blob/main/packages/mobx/__tests__/v5/base/make-observable.ts#L622-L1704 not (just) in typescript-decorators.ts
EDIT: this issue happens when using modern decorators
Intended outcome:
or
mobx
action
decorator should not implement a decorator forfield
if it's not supposed to workActual outcome: this test above fails, the fn
x
is not an actionHow to reproduce the issue:
Link to stackblitz with replication https://stackblitz.com/edit/vitejs-vite-hdtz1l?file=src%2Fmobx.test.ts
Versions mobx 6.12.3
Not sure why code in the 2022.3 decorator is marked as backwards/legacy and how it's supposed to work? It looks like it should be returning
return _createAction
or do more stuff inaddInitializer
to actually wrap the declared fn into an action?https://github.com/mobxjs/mobx/blob/bc1a7750fc24d9ad28f338cd0caf75bd504cbe94/packages/mobx/src/types/actionannotation.ts#L77-L82