Skip to content

Conversation

@markdalgleish
Copy link

I recently open sourced react-fetcher, a promise-based universal data fetching library. I noticed that your app basically uses the same pattern (including the separation between prefetching and deferring!), so I thought you might want to pull this functionality in from a library rather than embed it within the app.

What are your thoughts on this?

@erikras
Copy link
Owner

erikras commented Nov 12, 2015

Thanks, @markdalgleish! Hopefully we will soon be migrating to redux-simple-router. Can you evaluate how compatible this change will be with that?

@markdalgleish
Copy link
Author

Looks like I'll need to rework my PR once that's merged in. Also, the way that fetchData is called inside of createElement might require an API change on my part. I'll wait for the redux-simple-router changes to land and I'll update my work.

@erikras
Copy link
Owner

erikras commented Nov 12, 2015

Sounds like a plan. Thanks for this!

@mmahalwy
Copy link

@markdalgleish this is awesome! Excited for when it comes in

@bdefore
Copy link
Collaborator

bdefore commented Dec 31, 2015

me too! now that react-router has a 2.0 release candidate i'm hoping to make this play nice with react-simple-router ... @jlongster ?

@quicksnap
Copy link
Collaborator

Sorry this has taken so long to review--this looks good to me. A nice change =)

Could you resolve conflicts and let me know when it's ready to merge?

@andresgutgon
Copy link

How is related react-fetcher and async-Props?
Are both serving the same porpouse? In that case is not this PR imcompatible with this one #833?

/cc @sars

@bdefore
Copy link
Collaborator

bdefore commented Jan 17, 2016

@andresgutgon they do serve a pretty similar need. the decorators of react-fetcher fit nicely with the style of RRUHE, but async-props is in the react-router/rackt family, nicely integrated with the routing we use.

@andresgutgon
Copy link

So we must choose one of then. Right?

@sars
Copy link
Contributor

sars commented Jan 17, 2016

Guys, I thing both of them are not really good fit for this project to be honest.
react-fetcher is not integrated with router.
async-props is too huge and complex. There are a lot of logic there to pass data to props of our container. I think, it would be better to serve such data in redux state and connect it to component.

I almost finished async-props refactoring to better cover our needs.
I'm going to create new npm package....

@andresgutgon
Copy link

Wouldn't it be better to do a PR to async-props instead of doing a new package. I'm saying that because async-props is part of the rackt family and also it's related with redux. I don't know

@sars
Copy link
Contributor

sars commented Jan 17, 2016

I would propose to make interface like that:

@deferConnect({
  lunch: (params) => client.get('/lunches/' + params.lunchId)
})
@connect(state => ({auth: state.auth}))
export default class LunchDetails extends Component {

It will block rendering until promise (or several promises) is completed
It serves data to redux state.
It dispatches actions about loading process.
It connects result of promise and some other useful state data to this.props.lunch
Something like:

{loading: false, loaded: true, data: {id: 1, name: 'whatever'}}

You have this data in your redux state, so you can connect this date to another components as well
It allows you to keep everything in one file, do not create new action creators, reducers.

What do you think?

@sars
Copy link
Contributor

sars commented Jan 17, 2016

@andresgutgon It's too complex to my mind... And the main idea is to pass async data to props. I really prefer to keep data in redux state. It really simplifies this lib and provide you more flexibility

@andresgutgon
Copy link

I like your idea @sars

@sars
Copy link
Contributor

sars commented Jan 18, 2016

Guys, have a look please:
https://github.com/Rezonans/redux-async-connect

So, how it works:

Server integration is really similar:
https://github.com/sars/appetini-front/blob/appetini/src/server.js#L89

Client integration too:
https://github.com/sars/appetini-front/blob/appetini/src/client.js#L22

Notice - you can pass any helpers, that could be useful for you in decorator

You need to connect reducer as well:
https://github.com/sars/appetini-front/blob/appetini/src/redux/modules/reducer.js#L29
It MUST be in reduxAsyncConnect key

That's it... Now you can connect you data:

https://github.com/sars/appetini-front/blob/appetini/src/containers/Home/Home.js#L13
https://github.com/sars/appetini-front/blob/appetini/src/containers/App/App.js#L20

They will be available in this.props in your component

Under the hood:
asyncConnect dispatches action with your data and connect them to your component.
You can connect to them in other components too, smth like: https://github.com/sars/appetini-front/blob/appetini/src/containers/App/App.js#L31

What do you think, guys?

@bdefore
Copy link
Collaborator

bdefore commented Jan 18, 2016

this looks great @sars! i'll have a look at bringing this into universal-redux shortly.

@andresgutgon
Copy link

@sars so now all redux async data like user lives under ReduxAsyncConnect namespace?

@sars
Copy link
Contributor

sars commented Jan 18, 2016

Yes, 'reduxAsyncConnect'

@toxahak
Copy link

toxahak commented Jan 18, 2016

Wow, good idea, @sars ! No more unnecessary actions creators and reducers.
@andresgutgon As i understand, in such cases like user you can add you own reducers to store copy of received data everywhere you want.

@bdefore
Copy link
Collaborator

bdefore commented Jan 18, 2016

Hmm... that would end up mapping all of our async calls to reduxAsyncConect and not allow us to act on the responses directly? Rather than pass helpers to reduxAsyncConnect, would it be possible to wrap the promises we make in our existing reducers? such as in Widgets.js load method:

promise: (client) => client.get('/widget/load/param1/param2')

Which could become:

promise: (client) => reduxAsyncConnect(client.get('/widget/load/param1/param2'))

That way we could keep our existing logic for reducers? Alternatively, if this could export a formal Redux middleware, it could act as a replacement for ApiClient.

@sars
Copy link
Contributor

sars commented Jan 18, 2016

@bdefore You can listen for actions in your own reducers, if you want. And receive, modify and store data as you like...
In most cases you just need to store data from requests to state and that's it...

Could you give me some example?

@mmahalwy
Copy link

Forgive my ignorance, but @markdalgleish what is the difference between react-fetchr and async-props other than react-fetchr (as far as I know) has the ability to prefetch or to defer while async-props only prefetchs?

@sars
Copy link
Contributor

sars commented Jan 19, 2016

@mmahalwy, I made some comparison, have a look, please:
https://github.com/Rezonans/redux-async-connect#comparing-with-other-libraries

@jbrantly
Copy link

jbrantly commented Feb 6, 2016

FWIW, as a newbie trying to hook up SSR+react-router+redux for the first time, I really like the approach by @markdalgleish with react-fetcher (apparently now called redial). The main advantage I see is that it's super simple (it's basically just providing a mechanism to decorate your components and to query those decorations). Many of the related projects in this space seem to have been abandoned. I feel confident though that if redial is abandoned I could implement any bugfixes myself.

I understand that redial by itself does not automagically integrate redux+react-router, whereas redux-async-connect does, but the examples given in the README of redial do show how to do that integration.

Anyways, just thought I'd share my 2-cents as someone fresh to these concepts.

@sars
Copy link
Contributor

sars commented Feb 7, 2016

@jbrantly redux-async-connect is really simple , see:
https://github.com/Rezonans/redux-async-connect/blob/master/modules/ReduxAsyncConnect.js
main component includes only 130 lines of code ...

@markdalgleish
Copy link
Author

I've recently renamed react-fetcher to redial and changed the API. I'm closing this PR as it seems like async-props is getting more attention in #833. I'm happy to reopen this later and update if requested.

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

Successfully merging this pull request may close these issues.

9 participants