React Router integration #128

alex35mil · 2015-10-21T21:51:13Z

NB! This branch will work only with these changes in gem: shakacode/react_on_rails#68

When changes will be applied to react_on_rails, React Router will be working with gem on the server and on the client.

But still there are 2 things, that needs to be handled by the gem on the Rails side:

Errors from router. We should send 500 in case of the errors.
Redirects from router. We should send 302 in case of the redirect.

@justin808 @samnang Please take a look at these TODOs. And comment on 2 statements above.

justin808 · 2015-10-21T22:29:34Z

client/package.json

@@ -49,9 +50,10 @@
    "loader-utils": "^0.2.11",
    "marked": "^0.3.5",
    "react": "^0.14.0",
-    "react-bootstrap": "^0.27.0",
+    "react-bootstrap": "https://registry.npmjs.org/react-bootstrap/-/react-bootstrap-0.27.2.tgz",


Unreleased?

No, this issue: react-bootstrap/react-bootstrap#1395
I can't do npm shrinkwrap with new version of history because of npm conflict with react-bootstrap -> history dependency.

samnang · 2015-10-22T03:22:59Z

@alexfedoseev should the redirect still in context of react-router? or it needs to be client(browser) redirect?

@justin808 As I remember we already catch any exception when we render component, right?

justin808 · 2015-10-22T05:07:46Z

@samnang Yes, we catch all errors and log them.
@alexfedoseev Can we simulate a likely error from react-router?

Maybe we just need to remove the TODO since we do catch and log errors already.

alex35mil · 2015-10-22T07:49:03Z

should the redirect still in context of react-router? or it needs to be client(browser) redirect?

We need to send server response with 302 redirect from Rails to new location (it will be returned by generator function).

As I remember we already catch any exception when we render component, right?

Yep, it'll be logged in Development, but in Production we should send 500 response in case of server error, don't we?

@justin808 @samnang

alex35mil · 2015-10-22T08:46:15Z

Actually I think we should send 500 with stack on errors from server in development just like Rails do on errors. @justin808 @samnang what do you think?

justin808 · 2015-10-22T18:27:05Z

Maybe react_on_rails should be configured to either throw or render errors when server rendering?

alex35mil · 2015-10-22T18:32:58Z

I think it's more harm than good to give an option to treat server error as success response.

samnang · 2015-10-23T03:44:54Z

I think we need to choose either throw or render errors, we don't have to support both and have an option. Too many options could be confusing.

I vote for throwing an exception with stack trace. It's easier for development.

We need to send server response with 302 redirect from Rails to new location (it will be returned by generator function).

@alexfedoseev I'm a bit confused here, because after we render the component on server for the first request to the server, then after that everything will navigate at client side which depend on react-router to render different components, so why do we need to be done at server in this context? /cc @justin808

justin808 · 2015-10-23T05:47:37Z

Sure. We can change:

Throw exception if we catch a JS exception.
Handle the routing request.
Note, we already return the results back as a JSON array, so it's easy to add a 3rd element.

alex35mil · 2015-10-23T10:08:32Z

@samnang Here is the use case:

Guest navigates to Profile Edit page. Guest can't access to this area and, before render the component, we check his status in transition hook. Inside the hook we declare that if user is not authorized -> redirect him to Login path. And the only thing that we have from the match function of React Router is info about redirect location. So we can't render anything, we have to send 302 redirect from server to the new location.

justin808 · 2015-10-23T17:56:40Z

@alexfedoseev We need to create some simple example in the tutorial, as well as something simple in the dummy apps to test against. I think real code will make this clear.

alex35mil · 2015-10-23T22:00:29Z

@justin808 @samnang Here it is: c23b58a

Here we have pre-check: method + hook

If user will try to navigate to path with such pre-check:

by clicking on link from client: he will be redirected by methods of History API
by sending GET request to the server: router will tell us "Hey guys! I checked some stuff and looks like we have redirect here. Here is the info about redirect location, please, handle it. Bye-bye <3!"

So next we need to handle such cases on the Rails server.

alex35mil · 2015-10-24T10:16:14Z

We discussed redirect thing with @justin808. Not sure how to return 302 redirect here, because we're in the middle of render cycle and we can't send 302 redirect from this point, because it's controller's thing. So let's stick with JS redirect for now. If someone has idea how to handle such case — let us know.

justin808 · 2015-10-24T19:22:31Z

@alexfedoseev We'll generated some JS in the rendering that sets window.location just like we generate JS to put server console messages.

justin808 · 2015-10-24T19:51:18Z

@robwise Can you give this change a try. I'll give you a hand.

alex35mil · 2015-10-24T20:07:27Z

Do we want to make it in shakacode/react_on_rails#68 or merge shakacode/react_on_rails#68 and start new PR?

justin808 · 2015-10-24T21:32:26Z

@alexfedoseev Your call. It seems backwards compatible. It would be nice get some basic docs. @robwise can commit to this branch, so we don't need to start a new one. I want to get this in for version 1.0.0. Maybe we should release rc-pre-2?

robwise · 2015-10-24T21:38:21Z

@justin808 @alexfedoseev I agree, committing to this would make more sense as really the tests and generators should go along with the code they relate to.

alex35mil · 2015-10-25T16:05:11Z

👍 @robwise Let's call tomorrow and discuss next steps in this PR.

robwise · 2015-10-25T17:15:13Z

Just talked to Alex briefly on Slack, I should be able to get the generators branch done today or tomorrow at latest and then I can incorporate this into a generator as well as add some integration tests.

samnang · 2015-10-26T02:41:49Z

@alexfedoseev Thanks for giving the scenario, but I'm a bit unclear because I feel all authorization should be on the server. If users try to navigate by changing browser url to unauthorized page, then Rails should check and redirect the request even they render the page. If the user know JS and try to change the rendering component or path in client only to access unauthorized sections/screens, we don't have to prevent it because even they see it, but they can't update it because all update requests will go to server and server will reject it. Those users are just attackers, so we don't treat them like normal flow. Or I'm missing something here?

@justin808 @robwise

justin808 · 2015-10-26T04:05:51Z

I think what @samnang is referring to is why would the code even get to server rendering if the server knows the person is not allowed.

alex35mil · 2015-10-26T12:39:13Z

I feel all authorization should be on the server.

Source of truth — yes. All — not always. Because in case of SPA we need auth mechanism on the client to render UI and send auth headers with requests to API. And it's really simple example. There can be more complex cases. In SPA there is a form with 3 sections. User can't access 2nd section until he's done with 1st. So we need to add hook to 2nd section with logic: if 1st section is ok -> render 2nd, else -> redirect to 1st. And it's really annoying to duplicate routes + render logic on the server and on the client (just to send redirect), especially when SPA has only one entry point in the server routes.

@samnang @justin808 @robwise

samnang · 2015-10-27T10:23:41Z

After had a chance to discuss with @alexfedoseev, It makes sense in a context of SPA.

justin808 · 2015-11-06T23:15:36Z

@MaMute -- you could pull this branch and have it refer to the branch on react_on_rails with the fix. This can inspire your simple test.

shakacode/react_on_rails#68

justin808 · 2015-11-09T21:37:23Z

client/app/startup/ServerApp.jsx

+  let routeRedirect;
+  let routeProps;
+
+  match({ routes, location }, (_routeError, _routeRedirect, _routeProps) => {


This runs synchronously because we are not setting up the app with dynamic routes.

This does not affect the react_on_rails gem because it does not support dynamic routes.

@robwise FYI -- note for you in the docs.

justin808 · 2015-11-23T11:03:17Z

This is definitely ready. See shakacode/react_on_rails#68.

You can pull this branch and the gemfile refers to the branch for react_on_rails.

justin808 · 2015-11-23T11:04:50Z

@alexfedoseev @robwise @jbhatab @ryanaip @rstudner @thiagoc7, please review.

alex35mil · 2015-11-23T15:30:05Z

LGTM 👍

justin808 · 2015-11-24T09:41:58Z

@alexfedoseev Please see last commit f279c57 and let use know what you think.

@robwise Please be sure to update the Gemfile before merging. Actually, once we release, we put the version of the gem at 1.1.0.

justin808 · 2015-11-24T09:42:43Z

@robwise you've got some test failures after your last push.

alex35mil · 2015-11-24T09:57:18Z

@justin808 @robwise Looks good! 👍

robwise · 2015-11-24T17:47:16Z

@justin808 Aren't those coming from yours (2feb2e7)?

* Updated to react_on_rails 1.1.0 * Update config to get non-minified server msgs * Support for server rendering with react-router * Implement active class for react-router links and use IndexLink and IndexRoute * cleanup code duplication

justin808 · 2015-11-26T20:11:46Z

Once @alexfedoseev gets a chance to 👍 on the refactoring of the RouterCommentScreen, NonRouterCommentScreen, CommentScreen components, we'll merge!

alex35mil · 2015-11-26T20:31:57Z

All good! 👍

React Router integration

josiasds · 2015-11-30T19:33:45Z

It looks like I've missed all the action here. Great job! 👍 @justin808 @alexfedoseev @robwise

coveralls · 2017-10-06T00:32:49Z

Coverage decreased (-12.4%) to 87.576% when pulling e463a67 on alex/react-router into 088fac1 on master.

This was referenced Oct 21, 2015

Add React-Router Support shakacode/react_on_rails#68

Merged

Demonstrate react-router #74

Closed

justin808 reviewed Oct 21, 2015
View reviewed changes

alex35mil mentioned this pull request Oct 21, 2015

Handle errors and redirects from React Router on server shakacode/react_on_rails#69

Closed

muloka mentioned this pull request Nov 3, 2015

Demonstrate react-router + redux #72

Closed

justin808 reviewed Nov 9, 2015
View reviewed changes

justin808 mentioned this pull request Nov 10, 2015

Improve README and link to the v1.0.0 generator results repo shakacode/react_on_rails#93

Merged

justin808 force-pushed the alex/react-router branch 2 times, most recently from 909db2c to 2bc312e Compare November 23, 2015 04:56

justin808 force-pushed the alex/react-router branch from bd74555 to 2feb2e7 Compare November 23, 2015 11:09

justin808 force-pushed the alex/react-router branch from f279c57 to 12c06f0 Compare November 26, 2015 10:59

Add react-router support incl server rendering

e463a67

* Updated to react_on_rails 1.1.0 * Update config to get non-minified server msgs * Support for server rendering with react-router * Implement active class for react-router links and use IndexLink and IndexRoute * cleanup code duplication

justin808 force-pushed the alex/react-router branch from f0dcd4d to e463a67 Compare November 26, 2015 20:10

justin808 added a commit that referenced this pull request Nov 26, 2015

Merge pull request #128 from shakacode/alex/react-router

546dddb

React Router integration

justin808 merged commit 546dddb into master Nov 26, 2015

justin808 deleted the alex/react-router branch November 26, 2015 21:11

React Router integration #128

React Router integration #128

Conversation

alex35mil commented Oct 21, 2015

justin808 Oct 21, 2015

Choose a reason for hiding this comment

alex35mil Oct 21, 2015

Choose a reason for hiding this comment

samnang commented Oct 22, 2015

justin808 commented Oct 22, 2015

alex35mil commented Oct 22, 2015

alex35mil commented Oct 22, 2015

justin808 commented Oct 22, 2015

alex35mil commented Oct 22, 2015

samnang commented Oct 23, 2015

justin808 commented Oct 23, 2015

alex35mil commented Oct 23, 2015

justin808 commented Oct 23, 2015

alex35mil commented Oct 23, 2015

alex35mil commented Oct 24, 2015

justin808 commented Oct 24, 2015

justin808 commented Oct 24, 2015

alex35mil commented Oct 24, 2015

justin808 commented Oct 24, 2015

robwise commented Oct 24, 2015

alex35mil commented Oct 25, 2015

robwise commented Oct 25, 2015

samnang commented Oct 26, 2015

justin808 commented Oct 26, 2015

alex35mil commented Oct 26, 2015

samnang commented Oct 27, 2015

justin808 commented Nov 6, 2015

justin808 Nov 9, 2015

Choose a reason for hiding this comment

justin808 commented Nov 23, 2015

justin808 commented Nov 23, 2015

alex35mil commented Nov 23, 2015

justin808 commented Nov 24, 2015

justin808 commented Nov 24, 2015

alex35mil commented Nov 24, 2015

robwise commented Nov 24, 2015

justin808 commented Nov 26, 2015

alex35mil commented Nov 26, 2015

josiasds commented Nov 30, 2015

coveralls commented Oct 6, 2017