Skip to content

Conversation

@dzirtusss
Copy link
Contributor

@dzirtusss dzirtusss commented Aug 14, 2016

This change is Reviewable

@dzirtusss
Copy link
Contributor Author

Added isomorphic-fetch instead of axios. Draft.
Added temporary helper file ror.js -> future helpers that probably should go to ReactOnRails gem.

There is still a problem that isomorphic-fetch doesn't work in server rendering mode, so config.prerender = false

@dzirtusss
Copy link
Contributor Author

Some thoughts/reasons:

  • checkStatus() - needed because fetch presumes error checking and by default only raises error when some connection problems. W/o it catch will not work for ordinary errors. Also standard response.ok doesn't check for 304.
  • remaining of axios structures
.then(res => res.json())
.then(res => ({ data: res }))

Jsut left this way. Can keep like this or need to restructure app further down to remove data:.

  • makeJsonPostHeader() - kinda all-included POST. Need better naming.
  • maybe in the end it good to have 3 helpers getAuthenticityToken(), fetchJsonData() and makeJsonPostHeader()

@justin808
Copy link
Member

@dzirtusss We're not making any ajax requests when server rendering.

@justin808
Copy link
Member

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


client/app/bundles/comments/components/SimpleCommentScreen/SimpleCommentScreen.jsx, line 7 [r1] (raw file):

import BaseComponent from 'libs/components/BaseComponent';
import ror from 'libs/ror';

guessing this will go into ReactOnRails


client/app/bundles/comments/components/SimpleCommentScreen/SimpleCommentScreen.jsx, line 33 [r1] (raw file):

    return (
      fetch('comments.json')
        .then(res => ror.checkStatus(res))

Maybe these 3 lines should go into some library call?


client/app/libs/ror.js, line 34 [r1] (raw file):

      },
      body: JSON.stringify(entity),
      credentials: 'same-origin',

is this something new? the same-orgin part? Should ReactOnRails maybe make the whole header?


config/initializers/react_on_rails.rb, line 34 [r1] (raw file):

  # Default is false. Can be overriden at the component level.
  # Set to false for debugging issues before turning on to true.
  config.prerender = false

this should not be necessary


Comments from Reviewable

@dzirtusss
Copy link
Contributor Author

😕 Ok


Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


client/app/bundles/comments/components/SimpleCommentScreen/SimpleCommentScreen.jsx, line 7 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

guessing this will go into ReactOnRails

yes - what is in `ror.js` file can possibly go to gem

client/app/libs/ror.js, line 34 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

is this something new? the same-orgin part? Should ReactOnRails maybe make the whole header?

This is needed to pass session cookie to Rails inside request. It is fetch specific line.

For pure fetch compared vs axios it is needed much more configs like same-origin. checkStatus helper


config/initializers/react_on_rails.rb, line 34 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

this should not be necessary

Ok

Comments from Reviewable

@dzirtusss
Copy link
Contributor Author

@justin808

There are still 2 issues with isomorphic-fetch:

  1. doesn't work in server-render mode
  2. last PR Show errors on the "Simple React" page. #321 about error handling is dependent on axios responce.error and responce.error.data when rendering error message, which we don't have in fetch

What to do?

@justin808
Copy link
Member

@dzirtusss:

  1. We shouldn't be calling the fetch when server rendering.
  2. Please check with @AlexKVal on the 2nd issue.

@justin808
Copy link
Member

I'm open to reasons why or why this should not be merged (after rebasing on master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants