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

How to create a setter for an observable prop #839

Closed
sonaye opened this issue Feb 16, 2017 · 70 comments
Closed

How to create a setter for an observable prop #839

sonaye opened this issue Feb 16, 2017 · 70 comments

Comments

@sonaye
Copy link

sonaye commented Feb 16, 2017

I noticed a repeated pattern in my code setup, usually in my store i would have some observables defined as follows:

@observable loading: boolean = true;
@observable name: string = '';
// ..

And then for each observable, i would create a matching setter action:

@action setLoading = (value: boolean) => {
  this.loading = value;
};

@action setName = (value: string) => {
  this.name = value;
};

// ..

Later on in my components that have the store injected in them, i can easily access this.props.store.name and this.props.store.setName('foo').

This is not an issue when you have few observables, but for a large store with tens of observables, this becomes a tedious process and could lead to having errors when we wish to modify that pice of code, say the name of the observable for example. mobx should automate this for us i thought to my self, and indeed i found out about observable.box, although it requirers using .get() to retrieve the value of the observable, it would've been more convenient to have .get() automatically returned if .set() is not called.

A perfect api would be something like this:

loading: boolean = observable(true);
// or with decorators (more convenient)
@observable loading: boolean = true;
// or if it cannot be implemented directly for some reason
@observable @box loading: boolean = true;
// or just @box
@box loading: boolean = true;

// usage
console.log(this.props.store.loading); // prints true
this.props.store.loading.set(false);
console.log(this.props.store.loading); // prints false

Can this be done currently with mobx? did i miss something in the docs? if not, any thoughts on implementing something like this?

Coming back to react native, i've tried using boxed values:

test: string = observable('foo');
// also tried
test: string = observable.box('foo');

console.log(this.props.store.test);
this.props.store.test.set('voila!');
console.log(this.props.store.test);
// also tried console.log(this.props.store.test.get());

An error is being thrown in all cases:
simulator screen shot feb 17 2017 2 05 18 am

Please advise on this, thanks.

@sonaye
Copy link
Author

sonaye commented Feb 22, 2017

cc: @mweststrate

@mweststrate
Copy link
Member

mweststrate commented Feb 23, 2017

@sonaye why are you creating setters for each observable? What is the advantage of that? Note that each observable is already a box + some syntactic sugar. So @observable already creates a box for you. With a manually created box, as per documentation, you indeed need to read / write the value by using the .get() / .set() function.

Setters imho make only sense if you want to attach custom logic to specific observables (for which intercept could be used as well btw). To write a setter I would do the following:

class X {
   @observable private _x  = 3
   get x ()  { return this._x }
   set x (v) {
       // some logic
       this._x = v
   }

This issue further doesn't relate to RN

@mweststrate mweststrate changed the title Boxed Observables Not Working in React Native How to create a setter for an observable prop Feb 23, 2017
@jamiewinder
Copy link
Member

jamiewinder commented Feb 23, 2017

I sometimes find I have a setX for each x, just because I run in strict mode and sometimes need that granularity. However, I think actions are better off mirroring the kind of mutations your application actually makes... so for example:

@observable firstName;
@observable lastName;
@action setFirstName(value) {
    this.firstName = value;
}
@action setLastName(value) {
    this.lastName = value;
}

Do I ever set these individually? No, so it might as well be:

@observable firstName;
@observable lastName;
@action setName(first, last) {
    this.firstName = value;
    this.lastName = value;
}

Another example; a Rectangle model. I have an observable x, y, width, height. I could have a setX, setY, setWidth, setHeight if that made sense to my application... or maybe a single set action for all four... Or many setPosition and setSize. It could even be a parameterless autosize in some scenarios!

@sonaye
Copy link
Author

sonaye commented Feb 23, 2017

@mweststrate I create setters because they are needed to mutate observables, aren't they? how else would I update the value, or in the first place why would one create an observable if it was not changing? I don't mutate the data directly outside the store like this this.observable = 'bar';, instead i wrap it in a function/setter/action to ensure consistency across the app.

I understand that piece of code you provided, it's now clear to me that i misused the api (maybe we can polish the docs on that a little) and it's perhaps not rn related. What I am proposing is that by default mobx would define the setter attached to the observable as set x (v) { this._x = v }, and unless it was overwritten by a custom logic as you pointed out, this would be the default behavior. This saves us the headache of writing some repeated boilerplate. mobx is is all about automating things for us, and it's already doing an excellent job, i think this minor update would make a significant impact. Just by having this I would delete +250 lines of code of an application that I am working on as we speak.

I am proposing that just by writing one line @observable hello = 'world!'; you can directly read the value from the store store.hello and set it store.hello.set('new world!');

Instead of:

@observable private _hello  = 'world!';

get hello ()  {
  return this._hello;
}

set hello(value) {
  this._hello = value
}

get() is already automated, set() is not. What is your view on adding this? #69 could be related.

@sonaye
Copy link
Author

sonaye commented Feb 23, 2017

@jamiewinder I use strict mode too. You can still use the actions approach for multi observable updates. .set() should behave as an action anyway so that we can benefit from the transaction opmz.

instead of

@observable firstName;
@observable lastName;

@action setFirstName(value) {
  this.firstName = value;
}

@action setLastName(value) {
  this.lastName = value;
}

// ..
console.log(`${store.firstName}  ${store.lastName}`);
store.setFirstName('lorem');
store.setLastName('ipsum');

we would have

@observable firstName;
@observable lastName;

// ..
const { firstName, lastName } = store;
console.log(`${firstName}  ${lastName}`);
firstName.set('lorem');
lastName.set('ipsum');

how beautiful is that?

@jamiewinder
Copy link
Member

jamiewinder commented Feb 23, 2017

@sonaye

store.firstName is a string-returning property, so you won't be able to have a set method on it, as far as I'm aware..?

Doesn't have all your observables settable defeat the point of strict mode? I always saw it as a way to intentionally limit data mutability to logical operations?

@sonaye
Copy link
Author

sonaye commented Feb 23, 2017

@jamiewinder it's not a string-returning property, store.firstName is essentially a get(), it would return whatever type that observable is being casted, boolean, objects, numbers, you name it.

Doesn't have all your observables settable defeat the point of strict mode?

On the contrary, I think it would enforce using strict mode even more, .set() should act just like an action, it's just saves you some boilerplate for a very common use case. I agree that not "all" observables need something like this, for that we could introduce a new higher order function @box test = 'something';.

Note that we could/should still define custom getters and setters by overriding the default behavior:

@box test = 'something';

get test () {
  return `${this.test} extra`;
}

set test(value) {
  this.test = `${value} custom`;
}

Not sure if this can be implemnted directly though, naming-wise.

@urugator
Copy link
Collaborator

I think that @sonaye basically seeks a way to easily introduce simple observable prop which is a part of a public API, which means that setting such prop should be automatically considered an action to satisfy a strict mode.
If this is the case I think the demand is quite justified - in plain JS you also doesn't have to introduce a setter just because the property is exposed to outside world.
So maybe there could be a way to specify that observable field is "atomic", so that it's setter should be automatically wrapped in action...
However I don't have a clue how to introduce such option into the current modifiers/decorators design...
I also wonder how common the use case is.

@jamiewinder
Copy link
Member

jamiewinder commented Feb 23, 2017

@sonaye @urugator

Isn't this what observable.box / observable.shallowBox already do (albeit without the action on set I think)? I don't see how you can avoid the .get() because if you want to be able to do myObject.age to get the value (i.e. a number) then you can't do myObject.age.set(30) because you'd be calling .set on the numeric value.

@urugator
Copy link
Collaborator

urugator commented Feb 23, 2017

I meant a setter of internal observable generated by MobX, not a .set proposed by @sonaye ...
Something like @atomic @observable prop = "value". The obtaining/writing the prop is the same as without @atomic
EDIT: But I don't like the fact that the only purpose of @atomic would be to satisfy strict mode, which is just too "strict" :) in this case...

@sonaye
Copy link
Author

sonaye commented Feb 23, 2017

@jamiewinder that's the tricky part :) naming conflict.

@sonaye
Copy link
Author

sonaye commented Feb 23, 2017

@urugator can you provide a usage example of the api you are proposing, i am not sure that i fully understand the idea. how would we get and set values with @atomic @observable prop = 'value';?

@urugator
Copy link
Collaborator

urugator commented Feb 23, 2017

Look, currently, when you do @observable prop = "value"; the Mobx replaces prop by getter (for subscription logic) and setter (for setting the value and notifying observers). The value itself (which is accessed by this generated getter and setter) is moved to some internal hidden property (something like this.$mobx.values.prop).
Obviously the generated setter doesn't wrap the modification of observable value into an action, but that's exactly what you want (I presume?). So the idea is to have an ability to instruct Mobx, that generated setter should be an action (which means that setting the value alone is considered an atomic operation).
In the end it would work the same way as following, just without the "unnecessary" boilerplate and without the ability to access the "nonatomic" _field):

@observable _field = "value";
@computed get field() {
  return this._field;
}
set field(value) { // setters of computed are automatically actions
  this._field = value;
}

// so the above could be replace by something like
@atomic @observable field = "value";

// accessing value is the same:
console.log(this.field);
// modifying the value is the same:
this.field = "value"; // always runs in action (satisfies strict mode)

The @atomic decorator is not an actual API proposal, it's just for illustration of the idea.

@hccampos
Copy link

I think this proposal completely defeats the purpose of strict mode and @action. Actions should represent exactly that: actions that the user performs and that should be traceable in the dev tools, etc. Having a simple way to just set values directly goes against this. If you have an action to set a single property, it should be marked and easily identifiable, just like any other action that is more complex.

To give you guys an example, in our codebase we make sure we don't even have any actions in our models. The models define the data we store and can contain methods to modify that data, but they contain no actions at all. All the actions go in services/stores that are called by the view. And they should have as little granularity as possible. When something goes wrong, we only have a few places that we need check in the services/stores to find the problem. If you try to modify the state directly MobX will throw.

Basically you get a nice one-way flux where your view renders the state, and then calls actions in the stores/services to update the state instead of modifying the state directly. This is similar to Redux but in a mutable way and without all the boilerplate: instead of an immutable state tree, action types, and action creators and all that, you call action methods that mutate the state and MobX takes care of the rest.

@benjamingr
Copy link
Member

If you're creating getter/setter pairs for everything your data is not being hidden anyway and mutations aren't guarded in a meaningful way.

Just turn off strict mode and remove all the dead code. You can always define a get x() getter property without needing a getX() method. You can still pass readonly copies of your data to places that don't need to be able to change it.

@urugator
Copy link
Collaborator

urugator commented Feb 24, 2017

@hccampos

I think this proposal

If you refer to what I have written, then again, just for claricitaion, it's not an actual proposal, it was an idea, I personally don't have a clear opinion about it...

defeats the purpose of strict mode and @action

I don't think so. Purpose of strict mode and @action is to avoid (mostly unintentional) inconsistend state modification.
In a strict mode, the Mobx automatically presumes that modifing a single observable can't be an atomic operation. When this is the case you're required to introduce a (quite verbose) code, which has no other purpose than to denote the operation as atomic.
Only difference here is how many lines of code it takes to denote the operation as atomic. In plain JS you say the prop is part of the public API so it's modifications are considered atomic. With MobX you have to introduce another private observable field, public setter and getter.

should be traceable in the dev tools

Don't see a reason why it shoudn't be traceable?

it should be marked and easily identifiable

It's not? Btw setters for computeds are automatically wrapped into actions and they itselfs aren't marked by @action (easily identifiable?).

All the actions go in services/stores that are called by the view.

I won't critize anything which works for someone. If it works, it's well done.
From the description provided, without any specific conclusions, I had these thoughts:
You had to introduce another layer of abstraction between your domain objects and view, which is not for free.
The domain model must be quite flat, most likely leading to something googlable as "transaction scripts" (and/or "anemic domain model" perhaps).

@hccampos
Copy link

hccampos commented Feb 24, 2017 via email

@mweststrate
Copy link
Member

I think @urugator ś @atomic is nice, but I doubt if it adds enough value to the core. Maybe mobx-utils? I think ones you go down the road of marking fields as atomic anyway, you are in almost the same situation as not using strict mode at all.

The interesting question is how to define setters. I don't think it cannot be made that much easier as it is now. For example @atomic(setterFn) @observable x = y, or even @observable(interceptorFn) x = y could be used. But on the other hand passing functions to decorators looks to me a lot more unclearer to read then just slamming a get / set around a private field? If it is too much typing, I think it can be easily solved by a user land / mobx-utils decorator?

@urugator
Copy link
Collaborator

urugator commented Mar 2, 2017

The interesting question is how to define setters.

I woudn't allow to define setter/getter. MobX already allows defining setters/getters the same ways as it's done in JS without MobX.
But MobX does't allow exposing observable field (without getter/setter in stric mode), which was the point of @atomic.

But I don't like the fact, it's just a fix for strict mode. I mean it's the strict mode which is flawed (limiting) here, not a missing functionality...

If I would need it so often and I couldn't live without strict mode, I would probably go with utility/user land decorator, generating the getter/setter from "private" field simply based on naming convention:

@public @observable _name = "value";

// => would generate
@observable _name = "value";
@computed get name() {
  return this._name;
} 
set name(value) {
  this._name = value;
}

or maybe allow something this?:

@computed("name") _name = "value";

@hccampos
Copy link

hccampos commented Mar 2, 2017 via email

@sonaye
Copy link
Author

sonaye commented Mar 2, 2017

I didn't know that computed setters are actions by default.

I like the idea of having this generated within a class by a one liner:

@observable _field = 'value';

@computed get field() {
  return this._field;
}

set field(value) {
  this._field = value;
}

let's say that we have an empty class:

class Store {

}

is there a way in javascript to inject that piece of code into the class? how would one approach this? in pure js (es6/es next)

something like:

class Store {
  inject(observableName: string, initialValue: any = null) {
    // javascript magic
  }
} 

const store = new Store();

store.inject('hello', 'world!');

which would be equivalent to:

class Store {
  @observable _hello = 'world!';

  @computed get hello() {
    return this._hello;
  }

  set hello(value) {
    this._hello = value;
  }
}

const store = new Store();

anybody have any ideas on this? if this can be done, and since the support is already there, i don't see any further need to modify mobx-core.

@sonaye
Copy link
Author

sonaye commented Mar 2, 2017

+1 for having it as a decorator in mobx-utils, provided that it is indeed possible.

import { atomic } from 'mobx-utils';

@atomic('hello', 'world!')
class Store {
  
} 

or even better (not sure if can be done though):

import { atomic } from 'mobx-utils';

class Store {
  @atomic hello = 'world!';
}

the naming of atomic is just to illustrate the concept.

@urugator
Copy link
Collaborator

urugator commented Mar 2, 2017

is there a way in javascript to inject that piece of code into the class?

Well with additional (or parametrized) decorator as suggested, but if you insist on method:

inject(observableName, initialValue = null) {
  const privateFieldName = "_" + observableName;
  extendObservable(this, {
    [privateFieldName]: initialValue,
    get [observableName]() {
      return this[privateFieldName]
    }, 
    set [observableName](value) {
      this[privateFieldName] = value;
    }
  })
}

@sonaye
Copy link
Author

sonaye commented Mar 2, 2017

@urugator did you test this? multiple errors are thrown computed getters and setters are not yet supported.

@urugator
Copy link
Collaborator

urugator commented Mar 2, 2017

No. What throws the error? Seems like a transpiler issue, try this:

inject(observableName, initialValue = null) {
  const privateFieldName = "_" + observableName;
  const descriptor = {};
  descriptor[privateFieldName] = initialValue;
  Object.defineProperty(descriptor, observableName,
    get: function() {
      return this[privateFieldName];
    }, 
    set: function() {
      this[privateFieldName] = value;
    }
  );
  extendObservable(this, descriptor);  
}

Haven't tested it either, but should be clear what it's supposed to do...

@sonaye
Copy link
Author

sonaye commented Mar 2, 2017

I think it's react native specific, babelHelpers.defineEnumerableProperties is not a function, it uses some custom babel helpers facebook/react-native#4844.

@sonaye
Copy link
Author

sonaye commented Mar 3, 2017

Full code:

// @flow

import Exponent from 'exponent';

import React from 'react';
import { Text, TouchableWithoutFeedback, View } from 'react-native';

import { extendObservable, useStrict } from 'mobx';
import { inject, observer, Provider } from 'mobx-react/native';

useStrict(true);

class Store {
  inject = (observableName: string, initialValue: any = null) => {
    const privateName = `_${observableName}`;
    extendObservable(this, {
      [privateName]: initialValue,
      get [observableName]() {
        return this[privateName];
      },
      set [observableName](value: any) {
        this[privateName] = value;
      }
    });
  };
}

const store = new Store();

store.inject('hello', 'Hello World!');

const App = inject('store')(
  observer((props: Object) => (
    <View style={{ alignItems: 'center', flex: 1, justifyContent: 'center' }}>
      <TouchableWithoutFeedback
        onPress={() => {
          props.store.hello = new Date().getTime();
        }}>
        <View style={{ padding: 20 }}>
          <Text>{props.store.hello}</Text>
        </View>
      </TouchableWithoutFeedback>
    </View>
  ))
);

const AppContainer = () => (
  <Provider store={store}>
    <App />
  </Provider>
);

Exponent.registerRootComponent(AppContainer);

The error is caused by store.inject('hello', 'Hello World!');, commenting the line allows a build, any way around this in react native?

simulator screen shot mar 3 2017 6 46 18 pm

@sonaye
Copy link
Author

sonaye commented Mar 4, 2017

I just solved this issue, required some minor tinkering in the react native packager.

I added the following snippet

babelHelpers.defineEnumerableProperties = function(obj, descs) {
  for (var key in descs) {
    var desc = descs[key];
    desc.configurable = (desc.enumerable = true);
    if ('value' in desc) desc.writable = true;
    Object.defineProperty(obj, key, desc);
  }
  return obj;
};

to https://github.com/facebook/react-native/blob/master/packager/src/Resolver/polyfills/babelHelpers.js

I will open an issue there to see if it is possible to extend this functionality with some sort of babel plugin or if not, switch to submitting a pull request.

That being said, the code works perfectly now and behaves as expected, the injection is valid, and reactions work, without compromising mobx stric mode. I think the remaining part would be to discuss the mechanism of how and weather this should be added to mobx-core or mobx-utils as described here #839 (comment).

@sonaye
Copy link
Author

sonaye commented Mar 4, 2017

@mweststrate
Copy link
Member

@urugator the idea here is that userland decorators just transform prop descriptors into new prop descriptors, and that the JS runtime applies the final chained result to the object / prototype. Because the JS runtime doesn't do that yet, Babel / Typescript generate code for that (Roughly Object.defineProperty(target, name, reduce(decorators))). So @computed is working as it should, @observable is the exception because it defines the property itself already, to work around some semantic differences between how Babel and Typescript actually apply decorators.

@mweststrate
Copy link
Member

Closing this issue for now. Good solutions can be found in the tread above, like the mobx-decorators pattern or the code snippets from the above. I don't think it should be part of mobx-core, if anyone wants to create a PR on mobx-utils feel welcome :)

@sonaye
Copy link
Author

sonaye commented Mar 14, 2017

snippet misses a small piece, the computed property is not actually defined

I couldn't get it to work, can you take a look at this fiddle @mweststrate and tweak it out to reflect the point you mentioned? for the sake of understanding, i am not well aquatinted with these.

@mweststrate
Copy link
Member

mweststrate commented Mar 14, 2017

This should do the trick:

https://jsbin.com/vusoyicoho/1/edit?js,console

mobx.useStrict(true);

const box = (setter) => {
  return  (target, name, descriptor) => {
    const privateName = `_${name}`;
    mobx.observable(target, privateName, descriptor );
    return mobx.computed(target, name, {
      get: function() { return this[privateName] },
      set: function(value) {
        this[privateName] = setter(value);
      }
    });
};
}
      
  

class Store {
  @box(v => v.toUpperCase()) hello = 'Hello World!';
}

const store = new Store();

mobx.autorun(() => console.log(store.hello));

store.hello = 'Hello MobX!';

@sonaye
Copy link
Author

sonaye commented Mar 14, 2017

^ Nice! one note though, the initial value of the observable is undefined on the first run, autorun should fire with the initial store values, shouldn't it?

It would also be nice to get rid of having the setter supplied within the decorator @box hello = 'Hello World!';, not suppling it breaks functionality. Is there a way around it?

@sonaye
Copy link
Author

sonaye commented Mar 14, 2017

isn't the context of this in get: () => this[privateName] (and set) wrong?

You were right about this @urugator, was just able to test it.

@mweststrate
Copy link
Member

mweststrate commented Mar 14, 2017

@sonaye ah the fiddle is off, the code snippet above is correct though (observable instead of extendObservable)

@mweststrate
Copy link
Member

@sonaye yeah you don't need the setter, you could default it to x => x, and then use @box(), if you want to be able to use @box as well, just check if the argument count > 1, and if so, call the decorator method immediately instead of returning it

@sonaye
Copy link
Author

sonaye commented Mar 14, 2017

@mweststrate are you sure about using observable instead of extendObservable?

Error: [mobx] Invariant failed: observable expects zero or one arguments

@mweststrate
Copy link
Member

See above, mobx.observable(target, privateName, descriptor );

@sonaye
Copy link
Author

sonaye commented Mar 14, 2017

Got it, thanks.

@sonaye
Copy link
Author

sonaye commented Mar 14, 2017

Final working implementation, tested on both web and react native, for future reference.

useStrict(true);

const box = (target, name, descriptor) => {
  const privateName = `_${name}`;
  observable(target, privateName, descriptor);
  return computed(target, name, {
    get() {
      return this[privateName];
    },
    set(value) {
      this[privateName] = value;
    }
  });
};

class Store {
  @box hello = 'Hello World!';
}

const store = new Store();

autorun(() => console.log(store.hello));

// these are mobx actions
store.hello = 'Hello JavaScript!';
store.hello = 'Hello MobX!';
store.hello = 'Hello Decorators!';

Yes, this is definitely better than extends EnhancedStore :)

jaredru pushed a commit to Remitly/react-native that referenced this issue May 9, 2017
Summary:
**Motivation**
detailed in facebook#12702.

**Test plan**
1) running [a piece of code](mobxjs/mobx#839 (comment)) that utilizes the helper without any modifications on the react native package results an error being thrown.

![b589a71c-0041-11e7-9d47-cb79efff3ba5](https://cloud.githubusercontent.com/assets/23000873/23579517/3c8fe992-0100-11e7-9eb5-93c47f3df3e0.png)

2) updating the list of helpers available by adding the definition of `defineEnumerableProperties` as provided by babel [babel/babel/packages/babel-helpers/src/helpers.js](https://github.com/babel/babel/blob/master/packages/babel-helpers/src/helpers.js#L275-L285) results no errors.

![kapture 2017-03-04 at 16 48 35](https://cloud.githubusercontent.com/assets/23000873/23579520/457b8ca0-0100-11e7-8ca4-c704c6e9631f.gif)
Closes facebook#12703

Differential Revision: D4658120

Pulled By: ericvicenti

fbshipit-source-id: 914aed4d313b3cc4f7ab99049d05d0aef269a3be
@cdeutsch
Copy link

cdeutsch commented May 30, 2017

I landed here because somebody wanted to introduce box in a large project I'm working on.

I don't think so. Purpose of strict mode and @action is to avoid (mostly unintentional) inconsistend state modification.

I review 100's of PRs and I completely disagree.

For me the whole point of strict mode is so things that change observables are declarative. I echo what was said by hccampos

This is a clear indicator something might trigger re-renders and is similar to other Flux patterns.

store.setHello('Hello MobX!');

This just looks harmless and is very difficult to follow when reading code.

store.hello = 'Hello MobX!';

If you're working on a large project with other developers, I'd encourage you to think hard before using box.

@urugator
Copy link
Collaborator

urugator commented May 31, 2017

things that change observables are declarative

Actions are not declarative by themself, it's decorator which makes them declarative.
Actions are simply a neccessity to allow safe state transitions from one consistent state to another consistent state. They are not always required, but because they ensure this safety and there aren't any serious reasons to not to use them, it makes sense to wrap any state modification into an action.
Strict mode was simply introduced so that you can be sure, that you haven't forgotten to use the action anywhere and therefore state consistency shouldn't be (unintentionally) compromised.
Introduction of @box wouldn't change anything about that, therefore it would not:
"completely defeat the purpose of strict mode and @action".

is similar to other Flux patterns

I wonder how, because flux states that stores should be invoked indirectly through dispatcher and I don't think it really cares whether setters or methods are used for anything.

this is a clear indicator something might trigger re-renders

I don't see how method invocation makes clear, that something might re-render?
And why should consumer care, whether something re-renders or reacts to state modifications in any way?

This just looks harmless

Which operation is or isn't harmless depends on whether it's exposed as part of a public API. No need to make it more complicated.

very difficult to follow

Assigments are very difficult to follow, but method invocations are not...? Pardon me, but this is just silly, the only problem here is API inconsistency, but both can be followed just fine.

The things you're suggesting seems more reletad to consistency policies, than the actual need for @action+strict mode.
I think the main point of @box was to eliminate the need to introduce a field and two functions, when you only need something really trivial. Whether @box should generate setSomething/getSomething instead of setter/getter is not so important from this perspective.

@cdeutsch
Copy link

I don't see how method invocation makes clear, that something might re-render?

Pardon me, but this is just silly, the only problem here is API inconsistency, but both can be followed just fine.

Have you used any flux patterns before?

I'm coming from Reflux. Every function available on a "store" was an "action" (not the same as a MobX action).

So calling a Flux "action" is a very intentional act that you're updating store state.

Similar to how you don't do this.state.lastName = "Bill" in a React Component you wouldn't do userStore.lastName = "Bill" in a Flux store.

For me, that's the value MobX Strict brings to the table. Maybe that isn't the original intention. I couldn't find any clear answer on "why" there is a MobX Strict mode but two 3rd party sources recommend it for what I'm talking about.

If you don't care about having a one-way data flow pattern like Flux, and are more interested in something that resembles data-binding, then box makes sense.

@hccampos
Copy link

I agree 200% with @cdeutsch. In flux, state can only change via actions that are dispatched via the dispatcher. In most implementations an action is just a plain object with a type and a payload. Stores then subscribe to those actions and update their internal state (in redux there is only one and it returns a whole new state).

https://github.com/facebook/flux/blob/master/examples/flux-concepts/README.md

The difference with MobX is that actions are just methods and you call them directly. Just doing that you are already losing some of the value that flux gives you (decoupling the store from the components as much as possible), but at least you still have these annotations telling you exactly what you can call and what can trigger state updates. You also get the nice logging in the console/dev tools because each action will usually have a nice name indicating what happened in the application.

That last part is immensely important because when debugging you don't want to see a log for every little property that got changed (for that you could just as well step through the code in the debugger). What you want is to see something like addTodo or filterTodo which internally can modify several things. Granted, there can be simple cases where you might want to just set a property like, say, todo.done but in that case it is way more explicit to have toggleTodo in your dev tools than some automatic name given by MobX. And really, how hard is it to have a toggleTodo method?

The idea that I am trying to pass here is that it is not just about making most things an action and making our lives easier in the short run by ensuring state consistency. It is about forcing us to stop and think about good method names and about which methods correspond to actual actions that happened in the application (caused by the user or coming from an api, etc) and then only mutating the state from those (hopefully few) methods.

@benjamingr
Copy link
Member

The difference with MobX is that actions are just methods and you call them directly. Just doing that you are already losing some of the value that flux gives you (decoupling the store from the components as much as possible), but at least you still have these annotations telling you exactly what you can call and what can trigger state updates. You also get the nice logging in the console/dev tools because each action will usually have a nice name indicating what happened in the application.

Not having that part decoupled (other than dynamic dispatch you get by having methods for free) is one of the reasons I love MobX - it means you get accurate stack traces that look just like plain method calls - sync debugging and real stacks/heaps when you try to figure out what's wrong rather than a log.

@urugator
Copy link
Collaborator

urugator commented May 31, 2017

every function available on a "store" was an "action"

No it wasn't. Flux actions invokes dispatcher, which invokes stores. The concept of flux actions exists only because of the need for dispatcher. The access to dispatcher must be somehow unified, so it's done by so called actions.
Flux says nothing about whether the actions should be dispatched via methods, setters, events or by anything else. It has nothing to do with the flux architecture.

annotations telling you exactly what you can call

Again, API tells what you can and cannot call. Annotations/decorators can make it easier to identify this API or hint what happens.
That's the property of annotations/decorators in general and apply to @box decorator as well. The only possible problem is incosistency.

You also get the nice logging in the console/dev tools because each action will usually have a nice name indicating what happened in the application

The @box also invokes action, so it can print any fancy name as well. Again only issue is inconsistency.

you don't want to see a log for every little property that got changed

Property annotated by @box is just a shortcut for setter with action. It's not a little property, it has the same significance as calling a method annotated by action. And that significance is not given by some decorator, but by exposing the prop/method to public.
However I don't think that logging should be an action's concern in the first place.

it is not just about

Quite different than: "completely defeats the purpose"

think about good method names

Definitely a good idea, I just claim it's not an @action / strict mode concern.

actual actions that happened in the application (caused by the user or coming from an api, etc)

To me one of the main benefits of MobX is the fact that I don't need any centralized dispatcher, therefore no concept of (flux) actions as well.
I can stick with the concept of methods and compose the objects like I am used to from OOP. The @action is simply there to make it work and I don't have to care where or how the action is called.

I don't suggest to use @box, I just think that the arguments don't apply here, at least not the way they are being put.
Using @box headlessly on every property would actually kill the point of strict mode, because you would declare every operation to be atomic, but notice you would still do that in declarative way and the flow would remain unidirectional. The MobX could resemble data-binding only when you wouldn't use action anywhere in any form, which is not the case.

@cdeutsch
Copy link

No it wasn't. Flux actions invokes dispatcher, which invokes stores

I was specifically talking about Reflux which cuts out the boilerplate of dispatching magic strings.

Again, API tells what you can and cannot call. Annotations/decorators can make it easier to identify this API or hint what happens.

That doesn't help when I'm deep in a code review on GitHub looking at a React Component. I don't want to have to "go to definition" to look that up and I definitely don't want to have to do it for every property exposed on a store.

Again only issue is inconsistency.

Inconsistency is a huge issue when working on a large project with lots of developers

I'm fairly certain the way I create "MobX Stores" to create a one way data flow is much different then the way you use MobX, so your arguments for using box so far are not swaying my opinion. We don't need to agree though. 😉

Whether it's worth trying to create one-way data flow when using MobX is probably the higher level question. Maybe it's not worth it, in which case box would be more appealing.

@urugator
Copy link
Collaborator

Inconsistency is a huge issue

Agree, but has nothing to do with flux architecture, strict mode or actions.

don't want to have to do it for every property exposed on a store.

Why should you? If you know it's exposed it must be safe to use. If you don't know whether it's exposed, you still have to go to definition/documentation.

so your arguments for using box

I haven't presented any arguments for using @box. I was just trying to disprove imho false arguments for not using it. I also don't use @box, but the reasons don't relate to flux or anything like it and are much more simple:

  • another concept, which doesn't worth introduction (to me)
  • incosistency
  • if you turn off strict mode, @box becomes completely useless, making it conceptually weird - you have to indroduce something (box) only because you have introduced something else (strict mode)

@sonaye
Copy link
Author

sonaye commented Jun 4, 2017

I've been using @box in production for over two months now. A couple of comments.

  • @box doesn't cause any inconsistencies in the flow of the state, it's a direct implementation of the mobx api, utilizing both @observable and @computed.
  • @box doesn't violate useStrict in any way, on the contrary, it enforces it. It can be confusing however for someone who is not familiar with the idea behind @box, store.hello = 'Hello JavaScript!' can be easily interpreted as a direct mutation to the store, when in fact with @box, it's not.
  • The only reason you would want to use @box is to have less boilerplate code, there are absolutely no reasons to introduce @box in your project other than that.

What kind of boilerplate code are you talking about?

class Store {
+  @box firstName = '';
+  @box lastName = '';
-  @observable firstName = '';
-  @observable lastName = '';
-
-  @action setFirstName = firstName => (this.firstName = firstName);
-  @action setLastName = lastName => (this.lastName = lastName);
}

const store = new Store();

console.log(store.firstName); // output: ''
console.log(store.lastName); // output: ''

+ store.firstName = 'John';
+ store.lastName = 'Doe';
- store.setFirstName('John');
- store.setLastName('Doe');

console.log(store.firstName); // output: 'John'
console.log(store.lastName); // output: 'Doe'

store.firstName = 'John' here is exactly equivalent to store.setFirstName('John') when defined as @action setFirstName = firstName => (this.firstName = firstName). It's a "mobx action", not a direct mutations to the store.

How is this useful?
When you have too many observables in your store, it becomes a tedious process to write an action for every observable that you have, @box automates this process.

@maxdeviant
Copy link

@sonaye @box seems like exactly what I'm looking for. Will have to check it out!

@devuxer
Copy link

devuxer commented Mar 30, 2018

@mweststrate ,

Quick question, I've been in the habit of using @computed even for simple property get accessors, but I notice in this example (which you put toward the top of this thread), you don't:

class X {
    @observable private _x  = 3
    get x () { return this._x }
    set x (v) {
        // some logic
        this._x = v
    }
}

My typical situation, by the way, is usually something more like this:

class X {
    @observable private _x  = 3

    get x () { return this._x }

    @action incrementX() {
       x++
    }
 }

In other words, I want get to be public but set to be private.

So, my question is, is it good practice or completely unnecessary to use @computed in this example?

class X {
    @observable private _x  = 3

    @computed get x () { return this._x } // is @computed necessary here?

    @action incrementX() {
       x++
    }
 }

Or is @computed only warranted if one or more observable props is manipulated in some way?

@urugator
Copy link
Collaborator

@devuxer Unless there is a real computation, the @computed is kinda useless (negligible memory/perf waste). However it's future proof in case the impl changes and mobx.isObservableProp(x, "x") will yield true which may be useful for debuging...

@devuxer
Copy link

devuxer commented Mar 30, 2018

@urugator,

Thanks for your reply! I'll guess I'll stick with it.

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

No branches or pull requests

10 participants