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 computed decorator does not apply action to setter #3882

Open
olee opened this issue May 23, 2024 · 4 comments
Open

New computed decorator does not apply action to setter #3882

olee opened this issue May 23, 2024 · 4 comments
Labels

Comments

@olee
Copy link

olee commented May 23, 2024

It seems that with the new decorators, setters for computed getters do not become actions anymore, as it is also mentioned in the documentation:

It is possible to define a setter for computed values as well. Note that these setters cannot be used to alter the value of the computed property directly, but they can be used as an "inverse" of the derivation. Setters are automatically marked as actions

The code at the bottom works fine with old decorators, but with will report the following error:

Since strict-mode is enabled, changing (observed) observable values without using an action is not allowed. Tried to modify: [email protected]

Also, it seems there is no way to use like an action decorator on the getter to intentionally make it an action.

How to reproduce the issue:

class Store {
    @observable
    accessor value = 1;

    @computed
    public get doubleValue() {
        return this.value * 2;
    }

    public set doubleValue(value: number) {
        this.value = value / 2;
    }
}

const store = new Store();
autorun(() => console.log(store.doubleValue));
store.doubleValue = 32;

See also:

function decorate_20223_(this: Annotation, get, context: ClassGetterDecoratorContext) {
if (__DEV__) {
assert20223DecoratorType(context, ["getter"])
}
const ann = this
const { name: key, addInitializer } = context
addInitializer(function () {
const adm: ObservableObjectAdministration = asObservableObject(this)[$mobx]
const options = {
...ann.options_,
get,
context: this
}
options.name ||= __DEV__
? `${adm.name_}.${key.toString()}`
: `ObservableObject.${key.toString()}`
adm.values_.set(key, new ComputedValue(options))
})
return function () {
return this[$mobx].getObservablePropValue_(key)
}
}

@Matchlighter
Copy link
Collaborator

Matchlighter commented May 30, 2024

(Implementation note/context - not really an answer) This is a little tricky with the 2023.x decorators spec. Getters and Setters may now have separate decorators applied to them - it's no longer the case that a decorator on one (eg getter) applies to the other (eg setter). The spec has grown enough that MobX may be able to workaround this now (using addInitializer), but I believe it'd have to redefine the property and would break some optimizations that occur in the JS engine. It would also increase integration issues with other (non-MobX) decorators.
An alternative would be to allow @action to be applied to setters. Less implicit, but probably more semantic?

@mweststrate
Copy link
Member

Setters are automatically marked as actions

This was a bad idea anyway, so probably better to keep it as is :). @action should be on the "outside", that is the event sources (on-click, websocket on-message etc), so that they span an entire "tick" of the application, not at the level of individual attributes where they are largely meaningless.

@olee
Copy link
Author

olee commented Jul 2, 2024

Then I'd suggest we allow applying @action to setters as well, because right now you'd have to do this:

    public set language(value: string | undefined) {
        runInAction(() => {
            this.#language = value;
        });
    }

And even if you don't intend to allow @action on setters, the documentation on the website should be updated imho.

@mweststrate
Copy link
Member

the documentation on the website should be updated imho.

agreed

because right now you'd have to do this:

Per above, the main guidance is that you make whatever calls this language setter an action, not the setter. One way to think about it: If MobX didn't allow nesting action, what is the most outer function where you could put the the action on? That is the right place. An action for an individual attribute is basically a pointless transaction. You want all assignments that need to happen to be wrapped in an action (which in cases can still be only one of course).

Since you cannot make in JavaScript a field assignment a first class callback of something, there should be a different place where putting action is more appropiate. For example: on-click={action(e ==> { store.language = "en_US" })}. This scales, because if you make that on-click handler more complicated, and add an another assignment to it, you now end up with a single transaction, rather than two, which would make MobX partially invalidate the dep tree of the signals also twice.

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

3 participants