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

Fix's a regression in ember-data >= 2.4.0 related to handleResponse changes #83

Conversation

jcope2013
Copy link
Contributor

@jcope2013 jcope2013 commented Apr 21, 2016

Fix's a regression in ember-data >= 2.4.0 related to arguments changing in RESTAdapter's handleResponse function emberjs/data#3930, pass along requestData as argument in active-model-adapter handleResponse override method as well as fix handleResponse - returns ajax response if not 422 response test

closes #82

…ng in RESTAdapter's handleResponse function emberjs/data#3930, pass along requestData as argument in active-model-adapter handleResponse override method as well as fix handleResponse - returns ajax response if not 422 response test
@jcope2013 jcope2013 force-pushed the broken-handleResponse-tests-ember-data-2.4 branch from c10cd47 to 03e8c44 Compare April 21, 2016 06:48
@jcope2013 jcope2013 changed the title Fix's a regression in ember-data >= 2.4.0 related tohandleResponse changes Fix's a regression in ember-data >= 2.4.0 related to handleResponse changes Apr 21, 2016
@@ -148,7 +148,7 @@ const ActiveModelAdapter = RESTAdapter.extend({
@param {Object} payload
@return {Object | DS.AdapterError} response
*/
handleResponse: function(status, headers, payload) {
handleResponse: function(status, headers, payload, requestData) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requestData isn't used within the method, so I wonder if we should update .jshintrc's setting for unused to change it from "vars" to true so JSHint errors for unused arguments too...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, I think unused: true is a really annoying setting. We often copy/paste from documentation as developers, and the unused argument gets minified out anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that is a good point.

@pangratz
Copy link
Contributor

Apart from my tiny nitpick about the not used argument this looks good, thanks @jcope2013

@clayreimann
Copy link

This bug is super annoying. The empty tests that are generated with a new model no longer pass.

ETA on this PR?

@pangratz
Copy link
Contributor

friendly pinging @fivetanley @bmac: can you take a look and merge if this looks good to you? I am 👍 on this.

@fivetanley fivetanley merged commit 5b1e4d9 into adopted-ember-addons:master May 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants