Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] Make TideComponent use keyPath prop instead of {...props} #22

Open
lwakefield opened this issue Jun 22, 2017 · 4 comments
Open

[wip] Make TideComponent use keyPath prop instead of {...props} #22

lwakefield opened this issue Jun 22, 2017 · 4 comments

Comments

@lwakefield
Copy link
Contributor

lwakefield commented Jun 22, 2017

My thoughts are still forming on this, so bear with me.

The outcome that I would like to achieve out of this is something like follows:

<Product productId={'asd'} /> // would render a product using global state calculated from productId

As far as I can tell, we cannot pass data from tide that is calculated based on props. The only option we have is to pass data from tide that is based on data from tide.

So with the above example, we would need to do something like <Product productData={...} /> where productData is passed down a number of levels.

I think this can be addressed, based on something similar to a connector described here. The general idea is that instead of passing props into a <TideComponent /> and using all props as the keypaths, you use a keyPath prop. Since keyPath might have some mapping fns, then you can use other props passed into the component as well. something like this.

One of the large issues with passing props all the way down, is that changing a prop at a higher level, forces updates all the way down the tree, which can be very expensive.

Is this already handled by mapProps? If this is the case, we should document that!

I am wondering if what the implications here are. I am not sure whether this is something that could be done in a backward compatible manner, but I will think about this some more.

I apologize for slightly rambling thoughts. I think a PR is the best way to demonstrate what I mean here!

@eldh
Copy link
Contributor

eldh commented Jun 26, 2017

Nice! I think it would be cool to support this use case.

There's no documentation on mapProps yet. It was added in 2.0 and I didn't get around to fixing the docs, will try to do that this week hopefully. I think we could address this in mapProps easily. Right now mapProps just takes the prop as only argument, but we could easily send all props as second argument. Then you could have a wrap + mapper like this:

const Product = wrap(UnwrappedProduct, {
  product: ['products']
},{
  product: (products, {productId}) => products.get(productId))
}

But other than bringing this functionality into tide core, it would be pretty easy to just compose React components to handle this case. Not sure what's best but I think we should try to avoid breaking changes at least.

@lwakefield
Copy link
Contributor Author

I really like the idea of being able to do:

const Product = wrap(UnwrappedProduct, {
  product: (state, props) => ['products', state.filter, props.id]
})

This way, you can calculate based off of state and props at the same time.

I would propose that before we consider adding this to the core project, we can achieve the same thing with HoC in projects that use tide. This way we can demonstrate whether it is useful/helpful/waste of time.

@eldh
Copy link
Contributor

eldh commented Jun 26, 2017

Yeah, a hoc sounds good 👍

@lwakefield
Copy link
Contributor Author

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

No branches or pull requests

2 participants