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

Allow factory to decide which props to re-use? #119

Closed
salmanm opened this issue Oct 18, 2016 · 5 comments
Closed

Allow factory to decide which props to re-use? #119

salmanm opened this issue Oct 18, 2016 · 5 comments

Comments

@salmanm
Copy link

salmanm commented Oct 18, 2016

As per Resolver.js#L151, it ignores existing props.

IMO resolver should not be smart in this regard. It should be upto the developer to decide if he wants to refetch the data based on some condition or just use existing data.

I'm using Redux, and my use case is the following.

@connect(({ users }) => { users: users.data })
@resolve('users', () => callApi('/users'))
export default class ...

The resolve factory is never called because of hasOwnProperty check. Currently, I'm doing a simple workaround by prepending an _ to the prop, which seems a bit dirty solution to me.

Since it already passes props to the factory, I would expect this check to be removed and let the factory decide if it wants to return the same data, or fetch it.

Thoughts?

@monder
Copy link
Collaborator

monder commented Oct 19, 2016

I'm not sure exactly if I understood the use-case.
react-resolver just resolves property if the component does not have it already,- which seems logical to me. If the property is defined already, it should be valid and up to date, if not - let the whole component re-render and fetch the data.

In your example, users property comes from the redux store, and you want for react-resolver to fetch the data if there is none. But how do you plan to set the data back to the store?
I'm not sure that mixing redux and react-resolver is needed here as it could be done just by using either one or the other.

By "prepending an _ to the prop" you mean in @connect? Like:

@connect(({ users }) => { _users: users.data })
@resolve('users', ({ _users }) => _users ? _users : callApi('/users'))
export default class ...

@salmanm
Copy link
Author

salmanm commented Oct 19, 2016

I'm using Redux as well as Resolver both. I put it in the Redux store on componentDidMount (at which point I already have the data loaded using Resolver). For subsequent requests I can read from Redux and skip the XHR.

Now, I am not sure if I should be decorating my component with connect first and then resolver, or the other way around (Let me know your opinion).

react-resolver just resolves property if the component does not have it already

But it checks it by hasOwnProperty, so even when the data is undefined in Redux store, the props will still haveOwnProperty, and the check will think the property already exist.

I am not against checking if the prop already exist, but I think it should be done within the factory. If you have a reason not to do so, then at lease we should check prop's availability by checking it to be non-undefined.

Makes sense?

@ericclemmons
Copy link
Owner

The scenario makes sense to me, but, when using Redux (or any other application-state manager), React Resolver may not fit in well with it.

I believe redial is the goto for this scenario.

Reason being, react-resolver was never intended to have to worry about the application changing (and re-re-resolving) unless the route/page changed.

I've personally done the "fake prop so I can dynamically change the real prop value" technique @monder has illustrated above, but I don't know that it's bringing clarity to your project vs. doing things the "redux way".

I hope this helps!

@monder
Copy link
Collaborator

monder commented Nov 23, 2016

Published v3.1.0 on npm. I suppose this issue could be closed as well.

@salmanm
Copy link
Author

salmanm commented Nov 23, 2016

Excellent! Thanks

@salmanm salmanm closed this as completed Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants