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

add redux saga #106

Closed
wants to merge 1 commit into from
Closed

add redux saga #106

wants to merge 1 commit into from

Conversation

somus
Copy link
Contributor

@somus somus commented Mar 24, 2016

Fixes #97

@mayankchd
Copy link
Contributor

mayankchd commented Apr 18, 2016

Sorry for being late on this. Any chance you can take a pull and solve merge conflict?. Happy to merge after that!

@carloscuatin
Copy link
Contributor

@somus you just need to update your repository and then do git push origin master --force and ready,
I am also happy that finally this request fusion extraction

@somus
Copy link
Contributor Author

somus commented Apr 19, 2016

Just noticed redux sagas breaks during server-side rendering since there are no promises returned from the fetch actions. I checked redux-saga repo, server rendering is not quite there yet. 🙁 But there are some ideas in this redux-saga/redux-saga#13. I will try to implement it.

@jforaker
Copy link

Just my two cents but as a huge fan of this project, I don't think this should be merged to master. Reason is plain Redux has a significantly easier barrier to entry for newcomers, and Saga is largely complex abstraction that is not necessary and does not provide any substantial improvements over the current architecture. I've shipped multiple production level Redux apps and even I cannot wrap my head around sagas, nor have I needed to. Also saying "fixes #97" is a bit of a smoke screen as it is not really an issue, its one person's request.

@somus
Copy link
Contributor Author

somus commented May 25, 2016

@jforaker Thanks for the input. What are you saying makes sense. Redux saga is quite complex. To keep the project beginner friendly, it is better not to add sagas. Do you think it is a better idea to have a separate project forked from mern for advanced users?

@carloscuatin
Copy link
Contributor

carloscuatin commented May 25, 2016

I would believe that we can handle mern-cli flags and if a project would be interesting for advanced users but this generated confusion for new, it is my humble opinion

@aaronmcadam
Copy link

I know this is an old PR, but the server-side rendering is broken because the saga functions don't return promises:

TypeError: consumer(...).then is not a function
    at runner (promiseUtils.js:11:10)
    at sequence (promiseUtils.js:20:10)
    at fetchComponentData (fetchData.js:11:10)
    at server.js:104:5

I think the solution is to separate the requests for the data needs from the saga, say a helper function, and use that in Component.need instead. What do you think?

@somus
Copy link
Contributor Author

somus commented Jun 20, 2016

@aaronmcadam This error comes because saga doesn't return a promise. One way to handle this situation is to make getPost function consuming requestUrl function to retrieve the posts which will return a promise. Use this function in your Component.need array, not the saga. You can use the same function for saga's yield. Let me know, if it works.

@aaronmcadam
Copy link

aaronmcadam commented Jun 20, 2016

Hi @somus, thanks for getting back to me! I ended up following the official redux-saga universal example, which runs the saga on the backend too. The example there doesn't need the need static anymore because data fetching all handled by the lifecycle methods - they dispatch the first action which kicks off the saga.

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.

5 participants