shouldResubscribe option

[damonmaria]

The problem

The main reason for this PR is I have a couple of cases where compose ends up in an infinite loop. It goes something like this:

1. On first call to my onPropsChange I start an async data fetch and fire onData which shows a contained component in one style
2. Later ,once data arrives, onData is fired again and the component changes look drastically
3. This seems to cause React to fire componentWillReceiveProps with the same prop values as before
4. react-komposer then calls my onPropsChange, and so we go back to step one
5. Repeat forever grinding that browser tab to a halt

See the note in the React docs of componentWillRecieveProps for an explanation that this being fired with the same props is expected.

At a higher level, sometimes (often) the lifecycle of the component has to be taken into account in onPropsChange. Many users will expect it to only fire once per component.

Previously there was no way to detect that onPropsChange is being called with the same props and the expensive / async operation does not need to be repeated.

What this PR does

1. The current !pure and !shallowEqual check for the props is done in componentWillRecieveProps before onPropsChange is fired, not just in shouldComponentUpdate. This will stop a lot of existing redundent calls to onPropsChange but might somehow cause a backwards incompatible change if current react-komposer users have impure onProposChange functions but are not setting pure: false (not very likely IMHO).
2. The above check can be overriden with a custom function passed in the options as shouldResubscribe. This allows the user to check for a change in only the props that affect the onPropsChange function. This is useful, for example, when there are multiple layers of compose components and props coming down from outer components then don't need to fire onProposChange on unrelated inner components.

   Other implementation paths not taken

I thought about binding this to the component so cache check info could be kept on the component and checked by onPropsChange but that breaks the pure function principle that this package aims for. Same with having the same cache info in a variable outside the onPropsChange function (that also has bugs in certain scenarios if mutliple components are used).

I also thought about passing the old props into onPropsChange but that function already has enough parameters and it didn't seem good separation of concerns.

Tests / docs

Plenty of tests and API doc added.

Also added env: "mocha" to .eslintrc otherwise ESlint bawked on beforeEach().

[damonmaria]

Just committed a further addition that allows you to pass a fixed value (treated as truthy) for shouldResubscribe, instead of a function.

[clayne11]

You pass in nextProps and context but you also need to pass this.context to the shouldResubscribe function, just like you do with this.props.

You also shouldn't add mocha to the top .eslintrc. You should add that specifically under the tests folder.

This looks great. Looking forward to having this functionality, I think it will improve performance significantly.

[damonmaria]

Agree about mocha in .eslintrc. Should have done that.

I thought about passing the old/current context into the function, but does componentWillReceiveProps get called when the context changes? Because if not then this.context and the context param will always be the same. That was my assumption.

[clayne11]

I'm pretty sure it gets called when context changes because otherwise context would never update.

[damonmaria]

I'll add an enzyme test and see

On Wed, 6 Jul 2016 at 10:25 Curtis Layne notifications@github.com wrote:

I'm pretty sure it gets called when context changes because otherwise context would never update.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kadirahq/react-komposer/pull/99#issuecomment-230620925, or mute the thread https://github.com/notifications/unsubscribe/ABk9r1s0TQhOBEHAk1z8jNOYiJVQ5gSTks5qStn2gaJpZM4JEqcX .

[damonmaria]

Now also allows checking of changes to context. Tests added and API doc updated.

[clayne11]

I think you should squash your commits and rebase off of master so that this is ready to get pulled in. Make it as easy as possible for @arunoda.

I can't wait for this to make it in, performance will be WAY better.

[damonmaria]

@clayne11 This is my first PR so still figuring my way through. Will give it a go today.

[clayne11]

Let me know if you get stuck and I can point you in the right direction.

[damonmaria]

@clayne11 I think the best way to learn is to muddle your way through. Each time I understand git a bit more and a bit less at the same time :)

Squashed and rebased. Ready to merge.

[clayne11]

Looks good to me. Hopefully @arunoda will pull this in soon!

Fixes #4 & #2

[damonmaria]

Hey @arunoda, I think this PR will solve a few different issues. There's thorough testing. Any chance of a merge?

[clayne11]

I've been using it in production and it's definitely fantastic. Please merge.

[markshust]

The ticket at https://github.com/mobxjs/mobx/issues/84#issuecomment-235652897 explains a good use-case for merging this in...

[clayne11]

We really need this merged @arunoda. I'm currently using @damonmaria 's fork in production because I absolutely need this functionality.

[clayne11]

Any movement on this @arunoda? This is a critical feature that we need to have pulled in.

[arunoda]

Yep. This is pretty needed. I'm going to start fresh with the 2.x targeting this problem with a cleaner API. Releasing pretty soon (This week) See: https://github.com/kadirahq/react-komposer/issues/123

[clayne11]

Can you please just merge this and release a minor version with it? It's a critical feature and this PR has been sitting here for over a month.

On Tue, Sep 27, 2016, 02:16 Arunoda Susiripala notifications@github.com wrote:

Yep. This is pretty needed. I'm going to start fresh with the 2.x targeting this problem with a cleaner API. Releasing pretty soon (This week)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kadirahq/react-komposer/pull/99#issuecomment-249775866, or mute the thread https://github.com/notifications/unsubscribe-auth/ACs0VHmj3SnB4M5s9678gj3f4hQtBR9rks5quLTFgaJpZM4JEqcX .

[arunoda]

With v2, we've a similar API. Check this.