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

[FEATURE ds-extended-errors] Make adapter error extendable and add more error types #3586

Merged
merged 1 commit into from
Mar 29, 2016

Conversation

tchak
Copy link
Member

@tchak tchak commented Jul 25, 2015

This is going to help with questions like #3493 or #3491

@btecu
Copy link
Contributor

btecu commented Jul 26, 2015

This could probably be labeled as [BUGFIX release].

@tchak
Copy link
Member Author

tchak commented Jul 26, 2015

Strictly this is not a bug fix. I am not sure where is the line of what we back porting in 1.13 series. @bmac you have an opinion?

@bmac
Copy link
Member

bmac commented Jul 26, 2015

I think we should treat it as a new feature and feature flag it for a future 2.x release. If we find after it is release that it would provide value and help with the transition for users on the 1.13 series we can always backport it in the future.

@bmac
Copy link
Member

bmac commented Dec 4, 2015

@tchak What is the status of this pr?

@lindyhopchris
Copy link
Contributor

@tchak @bmac can I also ask what the status of this is? This is really required because AdapterError is just too generic to handle effectively in route transitions. Plus because it doesn't give access to the HTTP status code, it's difficult to handle.

For info, I do think that AdapterError should have the original HTTP status code on it. The reason being that the JSON API spec says:

Error objects provide additional information about problems encountered while performing an operation.

The key word here being additional. There's no requirement for a server to send a response that does include error objects within the JSON body - a simple HTTP status response with no content is valid. I know you've said in #3491 that the original HTTP response should not leak into the application, but it seems to me that the status code is required information considering that there is no obligation under the spec for the server to return error objects in its response body.

@bmac
Copy link
Member

bmac commented Mar 27, 2016

@tchak Do you mind rebasing this pr and wrapping it in a feature flag?

@tchak tchak force-pushed the more-error-types branch 2 times, most recently from 700bc04 to 010d534 Compare March 28, 2016 23:12
@tchak
Copy link
Member Author

tchak commented Mar 28, 2016

@bmac done

@tchak tchak changed the title Add more adapter errors types [FEATURE ds-extended-errors] Make adapter error extendable and add more error types Mar 28, 2016
@@ -906,7 +910,18 @@ var RESTAdapter = Adapter.extend(BuildURLMixin, {
let errors = this.normalizeErrorResponse(status, headers, payload);
let detailedMessage = this.generatedDetailedMessage(status, headers, payload, requestData);

return new AdapterError(errors, detailedMessage);
switch (status) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be wrapped in a isEnabled('ds-extended-errors');?

@lindyhopchris
Copy link
Contributor

@tchak @bmac

are there any plans to get this out from behind a feature flag? if so, what release might that happen in?

@bmac
Copy link
Member

bmac commented Oct 24, 2016

Currently the only thing blocking this feature flag from being enabled is a lack of documentation. We need some more fleshed out API docs for all of the errors and a pr on the guides repo explaining the new errors.

I haven't had any time to add docs for this feature but @lindyhopchris if you are up for the challenge a pr updating the error's doc blocks to describe when they trigger (and their status codes) would be a great place to start.

@wecc
Copy link
Contributor

wecc commented Oct 24, 2016

Related: #3585

@lindyhopchris
Copy link
Contributor

@bmac happy to give it a go, will probably have some time later this week to look at it and submit a PR.

@sandstrom
Copy link
Contributor

sandstrom commented Nov 7, 2016

I've noted is that ember-ajax also cover 400 Bad Request[1] and 422 Invalid[2]. Should these two also be included? (I think that would be good)

Second, ember-ajax has error classes for AbortError and TimeoutError, two errors that doesn't map to any HTTP status codes. But having them represented would be useful.

In our app abort errors are quite common but we currently have no good way of handling them separately from other types of errors.

@alexlafroscia Do you have any input/thoughts on alignment between ember-ajax and Ember Data's error handling? (this PR)

[1] https://github.com/ember-cli/ember-ajax/blob/master/addon/errors.js#L194
[2] https://github.com/ember-cli/ember-ajax/blob/master/addon/errors.js#L181


EDIT: Either my brain was clouded when I wrote this, or the code has changed since. Regardless, there is already support for AbortError, TimeoutError and InvalidError (422). All awesome things! Extended errors is a great addition to Ember Data! ⛵️

@lindyhopchris
Copy link
Contributor

@sandstrom
Copy link
Contributor

sandstrom commented Jan 31, 2017

@lindyhopchris Thanks, you are right! Thanks for correcting me + this is great news! 😄

@lindyhopchris
Copy link
Contributor

ping @bmac

would be great to see this feature taken out from behind the feature flag. any update on when this might happen?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants