Use fetch instead of $.ajax #73
Conversation
| "immutable": "^3.7.4", | ||
| "imports-loader": "^0.6.4", | ||
| "imports-loader": "https://registry.npmjs.org/imports-loader/-/imports-loader-0.6.4.tgz", |
There was a problem hiding this comment.
Just curious. what for?
There was a problem hiding this comment.
This looks odd.
npm i --save imports-loader
There was a problem hiding this comment.
Looks like imports-loader was shrinkwraped.
npm uninstall -S imports-loader
# remove `npm-shrinkwrap.json`
npm i -S imports-loader
npm shrinkwrap|
@mapreal19 @alexfedoseev @samnang Let's make a call on if we should with fetch or maybe axios. We could put fetch in for now, as an example of it. |
|
I feel if the |
|
@alexfedoseev @mapreal19 What's your opinion? @samnang Not sure which one you're voting for. I'm pretty sure that axios would use fetch underneath. My vote is for a layer of abstraction on top of fetch. |
|
I don't see necessity of using both libs, they are doing the same thing in the end. And because of this I have no special preference over any of two modules. // actionCreator.js
import 'apiCall' from 'myLibs/apiCall';
apiCall(params).then(/* ... */);
// myLibs/apiCall.js (layer of abstraction)
export default params => {
// do smth with params...
// I don't think that it's a big difference what will be used here
return fetch/axios(modifiedParams);
} |
|
Totally agree with @alexfedoseev 👍 Unless we want to do something that other one will do it easier. |
|
I say use use |
|
Looks like entry: ['whatwg-fetch', './assets/javascripts/App'],
// now it should be available as `fetch` without `webpack.ProvidePlugin` craziness@mapreal19 Can you try this one? |
|
@alexfedoseev I'll give it a try. Also I'd prefer a layer of abstraction |
|
@alexfedoseev Seems to be working. Thanks! 👍 |
|
Can you check in Safari? Chrome & Firefox both supports |
|
Yep I checked it there and worked fine. |
|
👍 |
|
OK -- this looks good -- I'm going to test this out as well, but since both @alexfedoseev and @mapreal19 confirmed, I did the simple merge to master. Great job! |
| @@ -40,7 +40,8 @@ | |||
| "redux-promise": "^0.5.0", | |||
| "redux-thunk": "^0.1.0", | |||
| "sleep": "^3.0.0", | |||
| "webpack": "^1.10.5" | |||
| "webpack": "^1.10.5", | |||
There was a problem hiding this comment.
@alexfedoseev @mapreal19 I'm pretty sure some of these dependencies are not needed for the rails deployment. It would be good to see see if the hot only dependencies were moved to devDependencies.
The problem with the build is copied here:
1) Add new comment Horizonal Form behaves like Form submits form
Failure/Error: visit root_path
Capybara::Poltergeist::JavascriptError:
One or more errors were raised in the Javascript code on the page. If you don't care about these errors, you can ignore them by setting js_errors: false in your Poltergeist configuration (see documentation for details).
Error: Cannot find module "whatwg-fetch"
Error: Cannot find module "whatwg-fetch"
at http://127.0.0.1:49150/assets/application.js:14672 in webpackMissingModule
at http://127.0.0.1:49150/assets/application.js:14672
at http://127.0.0.1:49150/assets/application.js:14645 in __webpack_require__
at http://127.0.0.1:49150/assets/application.js:14665
at http://127.0.0.1:49150/assets/application.js:16345
Shared Example Group: "Form" called from ./spec/features/comments_spec.rb:36
# /home/rof/cache/bundler/ruby/2.2.0/gems/poltergeist-1.6.0/lib/capybara/poltergeist/browser.rb:323:in `command'
# /home/rof/cache/bundler/ruby/2.2.0/gems/poltergeist-1.6.0/lib/capybara/poltergeist/browser.rb:31:in `visit'
# /home/rof/cache/bundler/ruby/2.2.0/gems/poltergeist-1.6.0/lib/capybara/poltergeist/driver.rb:95:in `visit'
# /home/rof/cache/bundler/ruby/2.2.0/gems/capybara-2.4.4/lib/capybara/session.rb:227:in `visit'
# /home/rof/cache/bundler/ruby/2.2.0/gems/capybara-2.4.4/lib/capybara/dsl.rb:51:in `block (2 levels) in <module:DSL>'
# ./spec/features/comments_spec.rb:28:in `block (2 levels) in <top (required)>'
# ./spec/rails_helper.rb:43:in `block (3 levels) in <top (required)>'
# /home/rof/cache/bundler/ruby/2.2.0/gems/database_cleaner-1.4.1/lib/database_cleaner/generic/base.rb:15:in `cleaning'
# /home/rof/cache/bundler/ruby/2.2.0/gems/database_cleaner-1.4.1/lib/database_cleaner/base.rb:92:in `cleaning'
# /home/rof/cache/bundler/ruby/2.2.0/gems/database_cleaner-1.4.1/lib/database_cleaner/configuration.rb:86:in `block (2 levels) in cleaning'
# /home/rof/cache/bundler/ruby/2.2.0/gems/database_cleaner-1.4.1/lib/database_cleaner/configuration.rb:87:in `call'
# /home/rof/cache/bundler/ruby/2.2.0/gems/database_cleaner-1.4.1/lib/database_cleaner/configuration.rb:87:in `cleaning'
# ./spec/rails_helper.rb:42:in `block (2 levels) in <top (required)>'
CC: @dylangrafmyre
|
We just need to follow the README instructions when adding new node modules and remember to npm shrinkwrap. I have fixed this twice today. |
|
very nice Dylan! |
|
I'm glad I wrote down what to do, or else I'd forget! |
|
The good news is that with NPM 3, shrinkwrap.json will be automatically updated when you npm install a new module. Similar to gemfile and gemfile.lock |
|
I'm going to to roll back the heroku build. |
|
We need to handle what jquery_ujs does for the crsf: |
|
@mapreal19 @alexfedoseev Let's get the CRSF part in and then resubmit the PR. |
|
@justin808 I think it should be fixed in #79 Null session it's the default value http://api.rubyonrails.org/classes/ActionController/RequestForgeryProtection/ClassMethods.html |
|
See: http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf and https://github.com/rails/jquery-ujs/blob/master/src/rails.js#L60 @alexfedoseev @mapreal19 If we can't do something as seamless as jquery-ujs, then maybe the current solution is really the best with Rails? I fixed the Promise issue by wrapping the jquery deferred. Perhaps we could create a small npm library, possibly with the react_on_rails gem, to make this just as easy. @mapreal19 Given the documentation at http://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf, do you still believe that a null session is appropriate for the general case? |
|
@dylangrafmyre @samnang What's your opinion of the CSRF issue with respect to switching from jquery-ujs? |
|
We need to include |
|
Looks like |
|
@justin808 I believe in this case it would be. Rails is acting as an API, and that's Also here:
It that's not the case, maybe the token should live in the store @alexfedoseev ? |
|
Either we have to have render from rails layout view template Then in React, we will need somehow to read that value from either option above and assign it to request header @justin808 @alexfedoseev @mapreal19 |
|
We can read this from the header of the document. But it's not something justified by putting as some extra code for the call to fetch. The standard way in rails is to use $.ajax, and you just don't have to think. The only reason that this is not great is that the deferred is not a Promise. So this is more of a stylistic discussion, than "can we do it" discussion. The solution if we want to go this way is clearly to improve the react_on_rails gem.
@skv-headless @josiasds Any opinions? |
Until we are using cookie-based authentication, we are not well-baked API. And since Rails in this case is not API-only application, I'd prefer to keep CSRF protection with csrf token.
I think it's kinda monkey patching: https://github.com/rails/jquery-ujs/blob/master/src/rails.js#L93 It's similar to if we will expose globally "our" |
|
This is where the token is being added: https://github.com/rails/jquery-ujs/blob/master/src/rails.js#L210 |
|
Looks like if we will submit form not with /cc: @justin808 @samnang @mapreal19 |
|
@alexfedoseev I'm quite sure that just using $.ajax that the inclusion of the rails_ujs library monkey patches jquery to include the token. The remote option is for rails controls. |
|
@justin808 You're right, this magic is here (: So this is the case:
|
|
So basically it's kinda |
|
I found this way to make it work: https://gist.github.com/mapreal19/338356e2e7a3140c45bb But as I've discussed with @justin808 this is adding more complexity to a problem already solved. cc @alexfedoseev @samnang |
|
@mapreal19 This should be abstracted out into reusable helper, since it's a common addition to every request. So, how we're gonna solve this? Save token to cookie or get it from the DOM in /cc: @justin808 @samnang @mapreal19 |
|
@alexfedoseev I say we fit something into the react_on_rails gem so it's seamless. Maybe some new method like rails_fetch that wraps the native fetch and includes the csrf tokens. Or we use the jquery_ujs one, and wrap the deferred into a Promise. |
fetchinstead ofajaxfor HTTP requests #69