-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Comments
cc: @mweststrate |
@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 Setters imho make only sense if you want to attach custom logic to specific observables (for which 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 |
I sometimes find I have a @observable firstName;
@observable lastName;
@action setFirstName(value) {
this.firstName = value;
}
@action setLastName(value) {
this.lastName = value;
}
@observable firstName;
@observable lastName;
@action setName(first, last) {
this.firstName = value;
this.lastName = value;
} Another example; a Rectangle model. I have an observable |
@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 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 I am proposing that just by writing one line 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. |
@jamiewinder I use strict mode too. You can still use the actions approach for multi observable updates. 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? |
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? |
@jamiewinder it's not a string-returning property,
On the contrary, I think it would enforce using strict mode even more, 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. |
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. |
Isn't this what |
I meant a setter of internal observable generated by MobX, not a |
@jamiewinder that's the tricky part :) naming conflict. |
@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 |
Look, currently, when you do @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. |
I think this proposal completely defeats the purpose of strict mode and 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. |
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 |
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...
I don't think so. Purpose of strict mode and @action is to avoid (mostly unintentional) inconsistend state modification.
Don't see a reason why it shoudn't be traceable?
It's not? Btw setters for computeds are automatically wrapped into actions and they itselfs aren't marked by @action (easily identifiable?).
I won't critize anything which works for someone. If it works, it's well done. |
It is basically the Flux pattern which has immense advantages in terms of
code comprehension and finding bugs. In Redux, for instance, you know your
state can only change via a reducer that gets called by an action. And you
don't have an action for each property of the state that needs to change
because that would make finding what is happening harder. You only have
actions for broader events coming from the user, from a server, etc. If the
granularity is too fine and if things go across a bunch of files you are
better off just reading the code to go understand what is happening in your
application. For large applications with complex state it is essential that
you are not forced to go around digging into a ton of files seeing all the
places from which the state got mutated.
…On Feb 24, 2017 01:00, "urugator" ***@***.***> wrote:
@hccampos <https://github.com/hccampos>
I think this proposal
If you refer to what I have written, than 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 <https://github.com/action>
I don't think so. Purpose of strict mode and @action
<https://github.com/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 tracable?
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 <https://github.com/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).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#839 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADlcCp8IHlmKsWVU-aUvR7QWf45bjaLEks5rfh2agaJpZM4MDrB2>
.
|
I think @urugator ś 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 |
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 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"; |
Looks like you just want a "not-so-strict" mode. Maybe strict mode could
accept a level flag.
…On Mar 2, 2017 3:38 PM, "urugator" ***@***.***> wrote:
The interesting question is how to define setters.
I woudn't allow to define setters/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 <https://github.com/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";
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#839 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADlcCslbGZkKxqpJM_6Xd_e6q3ZjPgoRks5rhtRXgaJpZM4MDrB2>
.
|
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. |
+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 |
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;
}
})
} |
@urugator did you test this? multiple errors are thrown |
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... |
I think it's react native specific, |
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 |
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). |
helper source from babel can be found here https://github.com/babel/babel/blob/master/packages/babel-helpers/src/helpers.js#L275-L285 |
@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 |
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 :) |
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. |
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!'; |
^ 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 |
You were right about this @urugator, was just able to test it. |
@sonaye ah the fiddle is off, the code snippet above is correct though ( |
@sonaye yeah you don't need the setter, you could default it to |
@mweststrate are you sure about using observable instead of extendObservable?
|
See above, |
Got it, thanks. |
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 |
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
I landed here because somebody wanted to introduce
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 |
Actions are not declarative by themself, it's decorator which makes them declarative.
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.
I don't see how method invocation makes clear, that something might re-render?
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.
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. |
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 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 |
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 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. |
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. |
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.
Again, API tells what you can and cannot call. Annotations/decorators can make it easier to identify this API or hint what happens.
The
Property annotated by
Quite different than: "completely defeats the purpose"
Definitely a good idea, I just claim it's not an @action / strict mode concern.
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 don't suggest to use |
I was specifically talking about Reflux which cuts out the boilerplate of dispatching magic strings.
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.
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 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 |
Agree, but has nothing to do with flux architecture, strict mode or actions.
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.
I haven't presented any arguments for using
|
I've been using @box in production for over two months now. A couple of comments.
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'
How is this useful? |
@sonaye |
Quick question, I've been in the habit of using 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 So, my question is, is it good practice or completely unnecessary to use 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? |
@devuxer Unless there is a real computation, the |
Thanks for your reply! I'll guess I'll stick with it. |
I noticed a repeated pattern in my code setup, usually in my store i would have some observables defined as follows:
And then for each observable, i would create a matching setter action:
Later on in my components that have the store injected in them, i can easily access
this.props.store.name
andthis.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:
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:
An error is being thrown in all cases:
Please advise on this, thanks.
The text was updated successfully, but these errors were encountered: