Observable props of observer-ed component

[Strate]

see #124

[Strate]

@mweststrate this PR contains bunch of fixes to reactive props/state. Latest version works perfectly with my current project (with strict mode enabled), so, from my point of view, it is good to merge.

[mweststrate]

Thanks for finding and fixing this issue! Looking good. The automatic function conversion is a legacy concern indeed, I'll deprecate that in the next mobx version.

I was wondering whether we should actually do something with state. I think we can put that back to it's original check in SCU (return this.state !== nextState), since if you want observable state, you probably just use instance fields directly. Making state observable might actually break for people that rely on the async nature of setState.

I wanted to wrap this change up, and some related ones, once I am back from ReactiveConf (first week of november). Does that sound OK for you?

[mweststrate]

Probably can be released earlier if you need it earlier as official build. Anyway, things left:

• thing about state observable or not
• docs
• release notes

[Strate]

Does that sound OK for you?

I am OK to use pre-release build for now, so, it is not need to release it asap. So, devote yourself for ReactiveConf :)

Making state observable might actually break for people that rely on the async nature of setState

Does this PR breaks it? It defines setter for state prop, without any overloadings for setState.

if you want observable state, you probably just use instance fields directly

I am totally agree with you, but I want from library be as less surprise developer as possible. So, if developer refer to state from computed function, computation result should be invalidated on state changes (without this, developer will spent hours of debugging).

[Strate]

@mweststrate seems that travis build is broken: I've just added failing test case, which fails npm test command on local machine, but travis build is green.

[Strate]

@mweststrate I've changed the way how props/state observed.
I've reverted back your shouldComponentUpdate change, because if it always returns false and re-render triggers only with forceUpdate called by reaction on props/state modification, componentWillUpdate/componentDidUpdate callbacks called with same values on nextProps/prevProps.
I've made props/state observable via Atom class, and report about modification only if new props/state is not shallow equal with previous one.
But for now component rerenders twice: on standard React's update way and with reaction, because props are reactive.
Seems that there should be a check in reaction, if only props/state changed, do not force update component. I don't know how to do that.

[mweststrate]

Ha though questions! I'm gonna take my time to think this through in two weeks :) But if you find a nice approach in the mean time let me know!

[Strate]

I've already pushed the fix, done simply with boolean flag :) Just works, maybe there is a better solution :)

[mweststrate]

Thanks a lot for your efforts! If you want btw I can make you part of the MobX organization, that allows you to push directly to (branches) of the mobx repo's. And it looks nice on your profile ;-)

[Strate]

It would be nice;)

[Strate]

@mweststrate could you please allow me write access to mobx-react npm package? I want to publish latest version of this PR (as a new version of [email protected]). This is my npm account: https://www.npmjs.com/~eshenbrener

[mweststrate]

Done

Op do 20 okt. 2016 om 10:05 schreef Artur Eshenbrener <
[email protected]>:

@mweststrate https://github.com/mweststrate could you please allow me
write access to mobx-react npm package? I want to publish latest version
of this PR (as a new version of [email protected]). This
is my npm account: https://www.npmjs.com/~eshenbrener

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#136 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvGhJ1zw0f-9MaBSaHAHyXN900hCjTJks5q1yDegaJpZM4KYsGF
.

[mweststrate]

Make sur eto use a --tag !

Op do 20 okt. 2016 om 10:18 schreef Michel Weststrate <[email protected]

:

Done

Op do 20 okt. 2016 om 10:05 schreef Artur Eshenbrener <
[email protected]>:

@mweststrate https://github.com/mweststrate could you please allow me
write access to mobx-react npm package? I want to publish latest version
of this PR (as a new version of [email protected]). This
is my npm account: https://www.npmjs.com/~eshenbrener

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#136 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvGhJ1zw0f-9MaBSaHAHyXN900hCjTJks5q1yDegaJpZM4KYsGF
.

[Strate]

@mweststrate great, thanks. About your note for --tag. As for now there is 2 dist-tag on mobx-react package: 'dist-tags': { latest: '3.5.8', beta: '3.5.8-fix134' },, there is no observableprops tag. I want to publish head of this PR under 3.8.5-observableprops.1 version, without --tag option (because there is no dist-tag for that version).
Is it OK, or I need to publish it under beta (or even alpha) tag?

[mweststrate]

Hi @Arthur, build that are not intended for general consumption should be
tagged with anything but "latest", so that they won't be installed
accidentally for everyone. So update the "version" field in package json to
"3.8.5-observableprops.1", then run "npm run build && npm publish --tag
beta".

Op do 20 okt. 2016 om 11:31 schreef Artur Eshenbrener <
[email protected]>:

@mweststrate https://github.com/mweststrate great, thanks. About your
note for --tag. As for now there is 2 dist-tag on mobx-react package: 'dist-tags':
{ latest: '3.5.8', beta: '3.5.8-fix134' },, there is no observableprops
tag. I want to publish head of this PR under 3.8.5-observableprops.1
version, without --tag option (because there is no dist-tag for that
version).
Is it OK, or I need to publish it under beta (or even alpha) tag?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#136 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABvGhCS5KyX_lINzpDm3cNQsXPLvsKHBks5q1zTcgaJpZM4KYsGF
.

[Strate]

Published as [email protected]

[leaysgur]

Is this dep still needed..?

[Strate]

Oh, no, I've forgot about this import. Gonna to remove.

[mweststrate]

@Strate just testing this PR. Great work, thanks!

One question: Do we need to make state observable as well? After all setState is the only way currently to modify state in react, which will always trigger a rendering anyway? Although now state is optimized, where in the previous implementation any state change would trigger a re-rendering. Is that the intended change?

[mweststrate]

Closed in favor of #154

[Strate]

Do we need to make state observable as well?

Yes, because developer could use state in computed function.

Although now state is optimized, where in the previous implementation any state change would trigger a re-rendering.

In a fact, state is not optimized from re-render point of view. As you can see, while atom.reportChanged called, skipRender set to true, this is not actually skip render of component, it is only prevent double-render. 1st caused by reactive props/state, second caused by standard react mechanics. So, setState always trigger re-render, this is not broken. We could add test case for this.

[mweststrate]

@Strate what I thought indeed. could you add the test case? after that feature should be good to go

[mweststrate]

Preferably use the v4 branch

[alexhisen]

This is a potentially breaking change. I've submitted a PR with some doc updates that reflect this change but it may need to be more prominent. Here is a use case where this is a breaking change: https://alexhisen.gitbooks.io/mobx-recipes/content/observable-based-routing.html