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

Added recursive pick for withPropsOnChange feature #300

Closed
wants to merge 1 commit into from

Conversation

EloB
Copy link

@EloB EloB commented Jan 4, 2017

Hi

First of all I would like to thank you for this awesome library!

I often use the withPropsOnChange but I miss the functionality to listen for nested properties and writing functions looks totally ugly. So I came up with this.

withPropsOnChange(
  (props, nextProps) =>
    props.router.routeParams.something !== nextProps.router.routeParams.something ||
    props.somethingElse !== nextProps.somethingElse ||
    props.thisStartToLookUgly !== nextProps.thisStartToLookUgly,
  ({
    fetchSomething,
    router: { routeParams: { something } },
    somethingElse,
    thisStartToLookUgly
  }) => fetchSomething(something, somethingElse, thisStartToLookUgly)
)

The code looks even more ugly if the routeParams isn't defined with current implementation. You then need to have checks if it exists that bloats the code.

With my feature request you can replace this with:

withPropsOnChange(
  [
    ['router', 'routeParams', 'something'],
    'somethingElse',
    'thisStartToLookGood'
  ],
  ({
    fetchSomething,
    router: { routeParams: { something } },
    somethingElse,
    thisStartToLookGood
  }) => fetchSomething(something, somethingElse, thisStartToLookGood)
)

]),
{
one: '1',
'two\ndeep\nvalue': 'true',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seperator could be changed if we don't like the new line but property names probably won't include new lines (that is why I choose that character).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if we don't like this solution we could do this with a Map but then we have to add a new shallowEqual that supports Map.

}, [
'one',
['two', 'deep', 'value'],
['root', 'missed'],
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we try to go deeper ['root', 'missed', 'something'] would still return undefined.

@istarkov
Copy link
Contributor

istarkov commented Jan 4, 2017

Hi, thank you!
Can't say strongly no, but

Why not just use something like
withProps(({ router: { routeParams: { something } } }) => ({ something })),
before withPropsOnChange( it does not give a lot of overhead in number of code lines, or readability.

Also, I think that even I was one of the persons who saved this enhancer comment it's mostly overused.

I used it in every component I did, but now it's very rare enhancer in my code, javascript and React are fast enough to use it only in real "computational intensive" tasks. (Can say that such tasks are very rare in our codebase)

Also I dislike all that string getters in the code like get('x.y.z', a) or like in current recompose code, yes it makes code shorter, but in the near nice future (IMO ;-)) it will make impossible to use all that great features we see in editors of languages like C#, Java etc - I mean code refactoring (rename property), find all property usage, etc...

So from my POV we should not create or extend features non possible in strongly typed languages.

PS:
Also I wanna say if you use it for calling some asynchronous actions inside - not sure what means fetchSomething it's not the right place to do it, as there are few problems here - no cancellation provided, double renders, non sync state etc.

In my code I very rare use version of get similar to this gist

And write pick like constructions using modern js destructuring.

@EloB
Copy link
Author

EloB commented Jan 5, 2017

withProps(({ router: { routeParams: { something } } }) => ({ something }))

Will fail if either router or routeParams isn't defined. Sorry for short comment written on mobile 😁

@istarkov
Copy link
Contributor

istarkov commented Jan 5, 2017 via email

@EloB
Copy link
Author

EloB commented Jan 6, 2017

So I need 3 types of hocs then? That I will also call bloating the code 😂

@wuct
Copy link
Contributor

wuct commented Jan 10, 2017

The current signature of pick is same as lodash.pick and ramda.pick, so I think changing its signature here is no go.

So I need 3 types of hocs then? That I will also call bloating the code ...

I believe you meant using defaultProps. I seldom use it now. Instead, I use default parameters or ramda.path a lot. I find this approach makes my HOCs more readable. So, in your case, 3 HOCs can be reduce to 2 HOCs.

If it's still not good enough to you, one of the beauty of FP is that you can always compose a new function easily. Without changing the behavior of withPropsOnChange, you can create a function with a signature (keys: string[][]) -> (props: Object, nextProps: Object) -> bool and use it with withPropsOnChange.

@EloB
Copy link
Author

EloB commented Jan 10, 2017

@wuct So you also want another dependency? I don't like to have 1 000 000 dependencies in my package.json file. Feels like left-pad. This is just my opinion.

Default parameters in this case that is very common looks totally ugly and hard to read.

withPropsOnChange(
  (
    { router: { params: { something } = {} } = {} },
    { router: { params: { something: newSomething } = {} } = {} }
  ) => {
    // This example is for one prop. Think how multiple properties will look!
  }
)

@EloB
Copy link
Author

EloB commented Jan 10, 2017

@istarkov About that fetch claim. There are other ways of solving that.
For example https://github.com/plougsgaard/react-timeout or that fetch method itself could prevent it from running.

I often use withPropsOnChange with redux-api-middleware and use the bailout property. So never fetches data if it exists and it fetches data on demand on mount and when I change the route it automatically fetches new data and props will be passed to the component from redux state. That feels really like one direction code. I'm developing smart tv applications with crap javascript engines and on poor hardware. So please believe me when I say this that it's very performant! 😄

@istarkov istarkov closed this Jun 9, 2017
@EloB EloB deleted the feature/recursive-pick branch June 9, 2017 14:50
@bertho-zero
Copy link

Why pick does not support dot notation like lodash? it would be useful for onlyUpdateForKeys for example.

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

Successfully merging this pull request may close these issues.

4 participants