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

@action decorators on fields do not convert the field value into action #3879

Closed
Obi-Dann opened this issue May 21, 2024 · 15 comments · Fixed by #3902
Closed

@action decorators on fields do not convert the field value into action #3879

Obi-Dann opened this issue May 21, 2024 · 15 comments · Fixed by #3902
Labels

Comments

@Obi-Dann
Copy link
Contributor

Obi-Dann commented May 21, 2024

EDIT: this issue happens when using modern decorators

Intended outcome:

import { action, isAction } from 'mobx';

it('action field should be wrapped into mobx.action', () => {
  class Store {
    @action
    x = () => {};
  }

  const store = new Store();

  expect(isAction(store.x)).toBeTruthy();
});

or

mobx action decorator should not implement a decorator for field if it's not supposed to work

Actual outcome:
this test above fails, the fn x is not an action

How 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 in addInitializer to actually wrap the declared fn into an action?

if (kind == "field") {
addInitializer(function () {
storeAnnotation(this, name, ann)
})
return
}

@urugator
Copy link
Collaborator

urugator commented May 21, 2024

@Obi-Dann
Copy link
Contributor Author

Obi-Dann commented May 21, 2024

@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
image

@urugator
Copy link
Collaborator

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:
#3638 (comment)

@Obi-Dann
Copy link
Contributor Author

Thanks for the link, I fully agree with this comment #3638 (comment)

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 #3638 (comment)

cc @Matchlighter @Amareis @mweststrate

@Matchlighter
Copy link
Collaborator

@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 accessors 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.

@Obi-Dann
Copy link
Contributor Author

@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
image

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.

@mweststrate
Copy link
Member

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.

@Obi-Dann
Copy link
Contributor Author

@mweststrate I've got a PR with the fix for this issue #3883
Not sure this issue is related to #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

@jzhan-canva
Copy link
Contributor

jzhan-canva commented Jul 10, 2024

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() {...}
}

@jzhan-canva
Copy link
Contributor

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

@jzhan-canva
Copy link
Contributor

Hi @mweststrate, just wanted to kindly remind you to review PR #3902 whenever you get a chance. Thanks!

@urugator
Copy link
Collaborator

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

@Matchlighter
Copy link
Collaborator

@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.

@jzhan-canva
Copy link
Contributor

jzhan-canva commented Sep 11, 2024

Hi @urugator please find updated doc in my PR
also added a new unit test to demonstrate the behaviour

Matchlighter pushed a commit that referenced this issue Sep 16, 2024
)

* fix action on field

* Create soft-beans-dream.md

* cleanup

* update doc

* tidy up

* minor fix

* add doc and unit test
@urugator
Copy link
Collaborator

urugator commented Sep 16, 2024

@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

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