Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

mapPropsOnChange #60

Closed
istarkov opened this issue Nov 17, 2015 · 15 comments
Closed

mapPropsOnChange #60

istarkov opened this issue Nov 17, 2015 · 15 comments

Comments

@istarkov
Copy link
Contributor

recompose does not allow to use most used pattern similar to mapPropsOnChange

What if I need to create computationally intensive work only if some props is changed, but pass other props as is. So I get all props online, and compute results of intensive tasks only when it needed.

For example I have property x and data, I need to make some intensive calculus if data changed but also I need to get current version of x in my component. With mapPropsOnChange I get the value of x on the moment data has changed. But every time I use this pattern I need new x values.

This could be solved creating a HOC based on mapPropsOnChange but changing this line

https://github.com/acdlite/recompose/blob/master/src/packages/recompose/mapPropsOnChange.js#L23

on this

return createElement(BaseComponent, {...this.props, ...this.childProps})

What are you think about
HOC with name and signature

withPropsOnChange(
  depdendentPropKeys: Array<string>,
  propsMapper: (ownerProps: Object) => Object,
  BaseComponent: ReactElementType
): ReactElementType
@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

The propsMapper parameter of mapPropsOnChange() receives the entire props object, not just the dependent props. So you can achieve what you're looking for like so:

mapPropsOnChange(
  ['foo', 'bar'],
  ({ foo, bar, ...rest }) => ({
    foobar: computationallyIntensiveFunction(foo, bar) // computed prop
    ...rest // rest of props
  }),
  BaseComponent
)

@acdlite acdlite closed this as completed Nov 17, 2015
@istarkov
Copy link
Contributor Author

This will not work!
propsMapper will be called only if foo or bar has changed,
but if changed something in the rest you will got the old childProps

@istarkov
Copy link
Contributor Author

As I wrote the only way to make it work is to combine props with childProps at render

return createElement(BaseComponent, {...this.props, ...this.childProps})

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

Oh... I see what you mean now. Need to write a failing test case.

@acdlite acdlite reopened this Nov 17, 2015
@istarkov
Copy link
Contributor Author

But looks like mapPropsOnChange is the good name for current behavior,
it looks like for new behavior it will be better to create something like withPropsOnChange?

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

Actually, probably need to rethink mapPropsOnChange() entirely. I don't like mapping the entire props object, but then mixing it back into the original props object.

@istarkov
Copy link
Contributor Author

Sounds good, thank you!

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

I dunno about withPropsOnChange() either... could work.

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

Maybe we can avoid this confusion by having the propsMapper not receive the entire props object, only the dependent props. Then combine with the rest of the props at the end.

@istarkov
Copy link
Contributor Author

This will work. There is no need in computation to use nondependent props.

Like this?

import { Component } from 'react'
import pick from 'lodash/object/pick'
import omit from 'lodash/object/omit'
import shallowEqual from 'recompose/shallowEqual'
import createHelper from 'recompose/createHelper'
import createElement from 'recompose/createElement'

const mapPropsOnChange = (depdendentPropKeys, propsMapper, BaseComponent) => {
  const pickDependentProps = props => pick(props, depdendentPropKeys)
  const omitDependentProps = props => omit(props, depdendentPropKeys)

  return class extends Component {
    childProps = propsMapper(pickDependentProps(this.props))

    componentWillReceiveProps(nextProps) {
      if (!shallowEqual(
        pickDependentProps(this.props),
        pickDependentProps(nextProps)
      )) {
        this.childProps = propsMapper(pickDependentProps(nextProps))
      }
    }

    render() {
      return createElement(BaseComponent, {...omitDependentProps(this.props), ...this.childProps})
    }
  }
}

export default createHelper(mapPropsOnChange, 'mapPropsOnChange')

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

Yes except I'd change the implementation a bit so that

this.childProps = {
  ...propsMapper(pickDependentProps(nextProps)),
  ...omitDependentProps(nextProps)
}

@istarkov
Copy link
Contributor Author

You means props order, not moving omitted props into childProps?
(moving props into childProps we got something like old behavior)

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

Oh duh, yes

@acdlite
Copy link
Owner

acdlite commented Nov 17, 2015

Forget my previous comment

acdlite added a commit that referenced this issue Nov 17, 2015
dependent props. Resulting object is combined with non-dependent
props.

Closes #60
@istarkov
Copy link
Contributor Author

👍 Thank you!!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants