Fix #1518: Move root route configuration to routes.js, add documentation for creating a global saga #1545

ghost · 2017-01-30T23:19:03Z

Fix for issue #1518. The route configuration was previously split between app.js and routes.js. I've moved the App (rootRoute) configuration to the routes.js file, so it is all in one place. I also changed loading the root route to use getComponent, instead of component, to make it easier for people to implement a global saga if they want to do that. There is also new documentation on how to add a global saga. I've also updated the spacing for the route generators and tested them.

Original Pull Request Here: #1538

…ating a global saga (#1518)

gihrig · 2017-01-31T02:22:02Z

@RobertSheaO Thanks for cleaning up the PR! ✨

I'll be watching the discussion in #1518.

mihir0x69 · 2017-02-01T10:08:19Z

This is awesome. Quick question- why is there no saga in App/? I had to remove sagas and injectSagas(sagas.default); from the route config. Precisely, from here to get it to work properly.

I love the overall idea. Although, there seems to be a working solution for this particular problem, this PR solves two problems:

Creates a good, if not ideal, example for developers of implementing app-level sagas.
Unifies route configuration in a single file.

That being said, I haven't studied other implementations. There's a nice comment from @justingreenberg but that involves installing redux-saga-router.

I'd love to know what others think of this.

ghost · 2017-02-01T10:11:11Z

Ah, good catch. I'll update the PR with that change. As for the missing app/sagas.js, I didn't want to put it in the base app, as not everyone will need it.

…

On Wed, Feb 1, 2017 at 02:08 Mihir Karandikar ***@***.***> wrote: This is awesome. Quick question- why is there no saga in App/? I had to remove sagas and injectSagas(sagas.default); from the route config. Precisely, from here <https://github.com/RobertSheaO/react-boilerplate/blob/bb69de2e18fd690560f613bef001b1d606f8df37/app/routes.js#L65> to get it to work properly. I love the overall idea. Although, there seems to be a working solution <#1277 (comment)> for this particular problem, this PR solves two problems: 1. Creates a good, if not ideal, example for developers of implementing app-level sagas. 2. Unifies route configuration in a single file. That being said, I haven't studied other implementations. There's a nice comment <https://github.com/justingreenberg> from @justingreenberg <https://github.com/justingreenberg> but that involves installing redux-saga-router. I'd love to know what others think of this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1545 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6uhc4S8WSuO3yEova3t3tmWcQWeJv2ks5rYFmUgaJpZM4LyC7O> .

mihir0x69 · 2017-02-01T10:17:23Z

@RobertSheaO No problem! :)

as not everyone will need it.

True. But IMO, moving some of the repo-loading logic in the global saga would demonstrate its use in a better way. It'll be easiers for developers to understand and adopt this approach if they see the actual code.

ghost · 2017-02-01T17:04:05Z

@KarandikarMihir I agree that a built-in usage example would be great. Not sure about repo-loading, since that only happens on submit; however, I could perhaps add an example of preloading @mxstbr 's repos on app load. Thoughts?

gihrig · 2017-02-01T21:01:57Z

There's a nice comment from @justingreenberg

@KarandikarMihir The "nice comment" link got changed to JK's profile page ???

preloading @mxstbr 's repos on app load. Thoughts?

@RobertSheaO How about pulling the current GH star count for react-boilerplate?

ghost · 2017-02-01T21:55:11Z

Sounds good, I'll add that and update my PR

marcandrews · 2017-02-03T06:36:35Z

Nice work!

So another option for app-level sagas, and user authentication sagas, which I think this PR solves, is using a Provider similar to how containers/LanguageProvider is set up and utilized.

While app-level sagas could still be useful, I think at least for user authentication, a Provider is a better pattern/implementation.

Thoughts?

mihir0x69 · 2017-02-03T07:00:31Z

@gihrig Sorry! Here's the correct link

gihrig · 2017-02-03T08:29:34Z

There are a number of open issues surrounding difficulties with sagas and the way they are handled.

Ref: #1384, #1420, #1315, #1518, #1447 and this PR #1545.

I'm concerned about the lack of real-world experience behind these various proposals. The only two I know of having been applied are #1420 used in production by @adelsz and and #1447 used in production by @jeffbski, but not with react-boilerplate. @kopax has experience with #1447Comment and react-boilerplate. Perhaps he can update us with his experience so far.

I'm personally leaning toward #1447 (redux-logic) and possibly some combination of other proposals, but have not gotten far enough into this to make a firm stand. If anyone is interested, I have updated @jeffbski's fork to the 3.4 react-boilerplate release.

What I would really like to see, is for basic requirements of a simple addition to the example app to be drawn up, such as implementation of a simple login facility for example, then implemented in each proposal. I think the experience of those implementing the same set of requirements in the several proposed changes to RBP async handling would be enlightening.

more at #1554 - Thanks @marcandrews

ghost · 2017-02-03T20:17:27Z

I've been busy these past few days; however, I'll whip up an example to put into the example app. I'll also read over the above linked issues to see how they impact what I'm doing. Personally, I like the simplicity of this solution, since it uses sagas in the same exact way as all the other sagas. For my particular use case, this is how I am using them:

Every route, except my login route, has an onEnter property that checks for a user token.

routes.js

...
const { injectReducer, injectSagas, redirectToLogin } = getAsyncInjectors(store);
...
{
  path: '/',
  name: 'home',
  onEnter: redirectToLogin,
  getComponent(nextState, cb) {
  ...
  },
}

asyncInjectors.js

function redirectToLogin(store) {
  return (nextState, replaceState) => {
    // Look into store for token, if invalid, replaceState with the `/login` path.
}

Inside my App/index.js file that wraps the entire app, I check the token validity again, and if it's valid and I don't already have the data, I send the request to retrieve data using the global saga.

App/index.js

componentWillMount() {
  if (accessTokenIsValid) {
    if (!this.props.accountDetails) this.props.onLoadAccountDetails();
  }
}

ghost · 2017-02-03T20:21:22Z

@marcandrews See my above comment about using onEnter (react-router) for authentication. This happens at the earliest point in the app, the route config, which could potentially prevent executing unnecessary code for users who are not authenticated.

marcandrews · 2017-02-04T04:12:59Z

In the hopes of not detracting further away from @RobertSheaO and the awesome work he's done in this PR, I've created a separate issue #1554 in response to @gihrig's comment.

avdeev · 2017-03-29T12:18:40Z

docs/js/global-saga.md

+  // create reusable async injectors using getAsyncInjectors factory
+  const { injectReducer, injectSagas } = getAsyncInjectors(store);
+
+  childRoutes : [


childRoutes = [

avdeev

need to fix the doc

avdeev · 2017-03-29T12:25:41Z

docs/js/global-saga.md

+
+Like other sagas, your global saga will live within a container, the `App` container. Go ahead and add a file and use the following starter code; it's the same starter code for other sagas.
+
+######app/containers/App/sagas.js


###### app/containers/App/sagas.js

avdeev · 2017-03-29T12:25:54Z

docs/js/global-saga.md

+```
+
+Next we'll import the `sagas.js` file into our `App` route configuration by updating our `getComponent` function to asynchronosly load our files. You can just replace it with the one below.
+######app/routes.js


###### app/routes.js

magiccookie · 2017-07-14T16:18:32Z

please merge

mxstbr · 2017-07-14T18:03:33Z

Deploy preview failed.

Built with commit 2e57793

https://app.netlify.com/sites/react-boilerplate/deploys/59690735a700c4681563d721

coveralls · 2017-07-14T18:08:58Z

Coverage remained the same at 100.0% when pulling 2e57793 on RobertSheaO:dev into d292b17 on react-boilerplate:dev.

justingreenberg · 2017-07-21T07:42:01Z

@RobertSheaO thank you so much for your contribution :) as we have just upgraded to RR v4 and refactored asyncInjectors, this should be resolved (please see #1746) — i would love your feedback if you have the time to pull down dev branch and take it for a spin! thanks again

ghost · 2017-07-21T20:58:41Z

I’ll take a look. Thank you.

…

On Fri, Jul 21, 2017 at 12:42 AM Justin Greenberg ***@***.***> wrote: Closed #1545 <#1545>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1545 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA6uhYBg29lhDlmTboZhAZTH2jwAZo7Mks5sQFZOgaJpZM4LyC7O> .

circuitry2 · 2017-12-26T03:46:11Z

What happened to this? docs/js/global-saga..md no longer exists??? What is the recommended approach now?

lock · 2018-05-28T23:06:46Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Move root route configuration to routes.js, add documentation for cre…

d800e79

…ating a global saga (#1518)

ghost mentioned this pull request Jan 30, 2017

Move root route configuration to routes.js, add documentation for creating a global saga #1538

Closed

ESLint spacing cleanup

d099381

ghost changed the title ~~#1518: Move root route configuration to routes.js, add documentation for creating a global saga~~ Fix #1518: Move root route configuration to routes.js, add documentation for creating a global saga Jan 31, 2017

Patrick O'Connor and others added 2 commits January 31, 2017 12:17

Merge branch 'dev' into dev

97857ba

Update global saga documentation

bb69de2

Simplified routes.js config, and updated global saga instructions.

ec72339

marcandrews mentioned this pull request Feb 4, 2017

Yet another global- or App-level saga discussion :-/ #1554

Closed

jeremyadavis mentioned this pull request Mar 10, 2017

How to add saga for App container? #1636

Closed

avdeev approved these changes Mar 29, 2017

View reviewed changes

avdeev reviewed Mar 29, 2017

View reviewed changes

avdeev suggested changes Mar 29, 2017

View reviewed changes

avdeev reviewed Mar 29, 2017

View reviewed changes

gihrig mentioned this pull request May 23, 2017

Add a memoized getComponent wrapper so that components do not get reloaded #1734

Closed

7 tasks

janet mentioned this pull request Jun 8, 2017

Add logout flow janet/monkeys#33

Merged

gihrig mentioned this pull request Jun 11, 2017

Replace Redux Saga with Redux observable #1605

Closed

Merge branch 'dev' into dev

2e57793

justingreenberg closed this Jul 21, 2017

lock bot locked as resolved and limited conversation to collaborators May 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1518: Move root route configuration to routes.js, add documentation for creating a global saga #1545

Fix #1518: Move root route configuration to routes.js, add documentation for creating a global saga #1545

ghost commented Jan 30, 2017 •

edited by ghost

gihrig commented Jan 31, 2017

mihir0x69 commented Feb 1, 2017 •

edited by gihrig

ghost commented Feb 1, 2017 via email

mihir0x69 commented Feb 1, 2017 •

edited

ghost commented Feb 1, 2017

gihrig commented Feb 1, 2017

ghost commented Feb 1, 2017

marcandrews commented Feb 3, 2017 •

edited

mihir0x69 commented Feb 3, 2017

gihrig commented Feb 3, 2017 •

edited

ghost commented Feb 3, 2017 •

edited by ghost

ghost commented Feb 3, 2017

marcandrews commented Feb 4, 2017

avdeev Mar 29, 2017

avdeev left a comment

avdeev Mar 29, 2017

avdeev Mar 29, 2017

magiccookie commented Jul 14, 2017

mxstbr commented Jul 14, 2017 •

edited

coveralls commented Jul 14, 2017 •

edited

justingreenberg commented Jul 21, 2017

ghost commented Jul 21, 2017 via email

circuitry2 commented Dec 26, 2017

lock bot commented May 28, 2018


		Like other sagas, your global saga will live within a container, the `App` container. Go ahead and add a file and use the following starter code; it's the same starter code for other sagas.

		######app/containers/App/sagas.js

Fix #1518: Move root route configuration to routes.js, add documentation for creating a global saga #1545

Fix #1518: Move root route configuration to routes.js, add documentation for creating a global saga #1545

Conversation

ghost commented Jan 30, 2017 • edited by ghost

gihrig commented Jan 31, 2017

mihir0x69 commented Feb 1, 2017 • edited by gihrig

ghost commented Feb 1, 2017 via email

mihir0x69 commented Feb 1, 2017 • edited

ghost commented Feb 1, 2017

gihrig commented Feb 1, 2017

ghost commented Feb 1, 2017

marcandrews commented Feb 3, 2017 • edited

mihir0x69 commented Feb 3, 2017

gihrig commented Feb 3, 2017 • edited

ghost commented Feb 3, 2017 • edited by ghost

ghost commented Feb 3, 2017

marcandrews commented Feb 4, 2017

avdeev Mar 29, 2017

Choose a reason for hiding this comment

avdeev left a comment

Choose a reason for hiding this comment

avdeev Mar 29, 2017

Choose a reason for hiding this comment

avdeev Mar 29, 2017

Choose a reason for hiding this comment

magiccookie commented Jul 14, 2017

mxstbr commented Jul 14, 2017 • edited

coveralls commented Jul 14, 2017 • edited

justingreenberg commented Jul 21, 2017

ghost commented Jul 21, 2017 via email

circuitry2 commented Dec 26, 2017

lock bot commented May 28, 2018

ghost commented Jan 30, 2017 •

edited by ghost

mihir0x69 commented Feb 1, 2017 •

edited by gihrig

mihir0x69 commented Feb 1, 2017 •

edited

marcandrews commented Feb 3, 2017 •

edited

gihrig commented Feb 3, 2017 •

edited

ghost commented Feb 3, 2017 •

edited by ghost

mxstbr commented Jul 14, 2017 •

edited

coveralls commented Jul 14, 2017 •

edited