Make props & state observable in constructor & componentWillMount #496
Conversation
Moved instrumentation of state & props to `observer` decorator. Closes mobxjs#478
src/observer.js
Outdated
| @@ -177,17 +157,12 @@ function makeComponentReactive(render) { | |||
| * If props are shallowly modified, react will render anyway, | |||
| * so atom.reportChanged() should not result in yet another re-render | |||
| */ | |||
| let skipRender = false | |||
| this[skipRenderKey] = false | |||
There was a problem hiding this comment.
could we make this a non-enumerable prop?
There was a problem hiding this comment.
I think yes, of course
src/observer.js
Outdated
| makePropertyObservableReference.call(this, "props") | ||
| // make state an observable reference | ||
| makePropertyObservableReference.call(this, "state") | ||
| this[isForcingUpdateKey] = false |
There was a problem hiding this comment.
Please clarify, dont understood :(
There was a problem hiding this comment.
Sorry, making this a non-enumerable prop as well :)
src/observer.js
Outdated
| const valueHolderKey = Symbol(propName + " value holder") | ||
| const atomHolderKey = Symbol(propName + " atom holder") | ||
| function getAtom() { | ||
| return this[atomHolderKey] || (this[atomHolderKey] = createAtom("reactive " + propName)) |
There was a problem hiding this comment.
could we make this a non-enumerable prop?
There was a problem hiding this comment.
Yes. Will address this a bit later today.
| @@ -273,6 +248,32 @@ const reactiveMixin = { | |||
| } | |||
| } | |||
|
|
|||
| function makeObservableProp(target, propName) { | |||
| const valueHolderKey = Symbol(propName + " value holder") | |||
| const atomHolderKey = Symbol(propName + " atom holder") | |||
There was a problem hiding this comment.
I am wondering whether using Symbols is a breaking change. So far we didn't depend on them, but I think React itself already requires Symbol or a polyfill of it? (Since mobx-react 5 can still be used with mobx 4 so should work on any ES 5 env)
There was a problem hiding this comment.
I will check this out, maybe there is a way to do this without symbols.
There was a problem hiding this comment.
The support looks pretty good to me: https://caniuse.com/#search=symbol
There was a problem hiding this comment.
without symbols it could be done by putting an object under some ugly key. Symbols is preferred though :) You could always create a super naive polyfill like: function Symbol(name) { return ``$mobxReactProp${symbolname}${Math.random()}`` }. But since react itself already requires quite some polyfills (link, so I think we can bet on Symbol to be present. And otherwise fix it if somebody reports it
|
Looks like a great improvement! Left some questions |
|
Made internal props hidden, but keep Symbol as is. |
|
Merging! Will release soon :-) |
|
Wow, cool! Can't wait for release :) |
|
Released as 5.2.1! |
|
@mweststrate - Sadly I've just run into a problem that is 90% likely to be connected with the changes discussed here (got 5.2.1 installed just a few hours ago). |
|
@TomaszGrzelak oh, sorry for that. I gonna to make PR to naive polyfill Symbol |
Moved instrumentation of state & props to
observerdecorator.Closes #478