removing omit

[istarkov]

#104 (comment)

IMHO omit should be removed.

I'll explain why:

One of my most used construction with mapPropsOnChange was

mapPropsOnChange(
  ['a', 'b', 'c'], 
  ({...props, a, b ,c}) => ({
    ...props,
    someResult: blabla(a,b,c)
  })
),

spread operator allows me to not lost a,b,c in the future, and reuse them again.

Now to make this work I can't use spread operator at all, because other props out of selection array i.e. ['a', 'b', 'c'] will be placed in this.computedProps so will be outdated.

The code above after this PR now looks like

mapPropsOnChange(
  ['a', 'b', 'c'], 
  ({a, b ,c}) => ({
    a,
    b,
    c,
    someResult: blabla(a,b,c)
  })
),

So every time I add property into selection array I need to add it into object to preserve.

Removing omit will help a lot, I could just remove all that ...props, and forget about property omitting logic in mapPropsOnChange

Also this simplify this component logic, I got a lot of questions from coworkers about this omitting functionality of mapPropsOnChange

[codecov-io]

Current coverage is 100.00%

Merging #119 into master will not affect coverage as of 066c01e

@@            master    #119   diff @@
======================================
  Files           42      42       
  Stmts          312     311     -1
  Branches         0       0       
  Methods          0       0       
======================================
- Hit            312     311     -1
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 066c01e

Powered by Codecov. Updated on successful CI builds.

[hartzis]

Apologies for not responding to your comment in #104.

I'm all for this update. Thank you for putting this PR together.

I'm curious why the original omit was created? Maybe possibly making the assumption that since those props changed, you're computing something from them, and not going to need them after you've used them?

But you can just create your own omit props with mapProps.

[istarkov]

Original omit was created after this issue #60 it was a fast and working solution of that problem,
with minimal changes to that version of mapPropsOnChange.

[wuct]

The new behavior of mapPropsOnChange is closer to withProps rather than mapProps. Maybe we should rename it to withPropsOnChange?

[istarkov]

@wuct you are right, withPropsOnChange really good name.
@acdlite what are you think about?

[istarkov]

@acdlite please answer on this PR having this comment
#124 (comment)

As you see a lot of people and projects use this library, (I use it in every project now) and having React has changed (and only new recompose versions supports react 15 ) I need to have a decision what to do with mapPropsOnChange now.

[istarkov]

PS: I think it's not a big problem to write will or or not you to accept this,
be sure I'm heavily depends on your decision (to use local HOC version or to wait with library updates)

[acdlite]

I read your comment about mapPropsOnChange over here #124 (comment) but haven't fully digested it yet.

My main concern is that it's not clear to me (or most people, it seems) what mapPropsOnChange is actually supposed to do. Maybe it's just a problem of documentation or naming, but I suspect it's a problem of not properly identifying the use case and designing the API around that. This at the top of my list to figure out before we cut a 1.0 release.

If you're pressed for time, I would recommend using your own local version until we sort this out. Thanks for your patience :)

[istarkov]

Thank you, may be withPropsOnChange as supposed by @wuct will be the best choice. And yes mapPropsOnChange is not describe this component behavior.

[acdlite]

Okay I finally took another look at this. I think you're right @istarkov — removing omit clears everything up, and I think the name mapPropsOnChange still works, too.

[acdlite]

Merged! Thanks @istarkov!

I changed the behavior slightly... Instead of merging with the original props, it now acts like mapProps() and passes exactly what you return from the mapping function.

Will go out in the next release, soon.

[hartzis]

@acdlite I'm pretty sure that removing the merge of computed props into the original props will break components that currently expect props that are now lost because of the spread removal.

I agree with and like this update, just wondering if this is going out in v1.0.0?

It is a simple change, just need to add the ...rest in the function and then {...rest, ...computedProps} to the outputted props, but this change should be documented somewhere.

[istarkov]

Changed behavior will not work, as returning just computedProps you will get outdated results for props not in shouldMapOrKeys.

[wuct]

@istarkov is right, I have created a PR to show what he said.

[acdlite]

Blah you guys are right. Okay, let's add the merging back and rename it withPropsOnChange()