-
Notifications
You must be signed in to change notification settings - Fork 25
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
Handle backend errors deserialization + support RESTSerializer #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi :) It's a great feature! Thanks a lot for your contribution.
I have also a few remarks in comments, which we can, of course, discuss together.
README.md
Outdated
@@ -111,6 +111,7 @@ If you want to push the received data to the store, set this option to `true` an | |||
import { JSONAPISerializer } from 'ember-custom-actions'; | |||
export default JSONAPISerializer.extend(); | |||
``` | |||
If you use RESTSerializer instead of JSONAPISerializer to serialize your models, you don't need our extension of JSONAPISerializer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it's not true :/
You still have to use our custom serializer but for the RESTSerializer
which I forgot to implement :/
This is associated with feature flag emberjs/data#4110 which is not enabled by default yet.
I will add it in #16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it. Your RESTSerializer
is a nice addition. Hope this feature flag will become a feature. :)
addon/actions/action.js
Outdated
@@ -80,11 +80,13 @@ export default Object.extend({ | |||
|
|||
_promise() { | |||
return this.get('adapter').ajax(this.get('url'), this.get('requestType'), this.get('data')).then((response) => { | |||
if (this.get('config.pushToStore') && response.data) { | |||
if (this.get('config.pushToStore') && response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch :)
But we have to also handle a case when the response will be eg. String
or an empty Object
.
We should only allow to pushToStore
if the response is an Object
and pushToStore
option is true
This should be also fixed in #16 (it was highly related with RESTSerializer)
addon/actions/action.js
Outdated
return this.get('serializer').pushPayload(this.get('store'), response); | ||
} else { | ||
return response; | ||
} | ||
}).catch((errors) => { | ||
return this.get('serializer').extractErrors(this.get('store'), this.get('store').modelFor(this.get('modelName')), errors, this.get('model.id')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok :)
Could you move this.get('store').modelFor(this.get('modelName'))
to a let
varible?
I think we should also serialize errors only if pushToPayload
option was set to true
else we should return plain errors object as we do higher. But maybe we should introduce another flag for that?
We should also check if the errors
are the Object
class and have a key errors
(as spec say)
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember also about Ember.RSVP.rethrow(serializedErrors);
which will rethrow errors and will let us catch them in later stage instead of handling them in then
(as you know, the first argument in then
serve for success responses)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comments ! :) I made the updates. Does this sound good to you ? I just didn't figure out the rethrow thing.
README.md
Outdated
@@ -30,7 +30,7 @@ import Model from 'ember-data/model'; | |||
import { modelAction } from 'ember-custom-actions'; | |||
|
|||
export default Model.extend({ | |||
publish: modelAction('publish', { pushToStore: false }), | |||
publish: modelAction('publish', { pushToStore: false, extractInvalidError: false }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was thinking about it over the night and I think we should use pushToStore
flag also for errors serialization. Why?
In fact, the pushToStore
flag is responsible for serializing response which pushes to store results and returns models objects. The keyword here is "serializing response", that why I think this should be also responsible for serializing response errors.
Maybe in future, we should consider renaming this flag to serializeResponse
and make a deprecation for pushToStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this:
_promise() {
return this.get('adapter')
.ajax(this.get('url'), this.get('requestType'), this.get('data'))
.then(this._onSuccess.bind(this), this._onError.bind(this));
},
_onSuccess(response) {
if (this.get('config.pushToStore') && this._validResponse(response)) {
return this.get('serializer').pushPayload(this.get('store'), response);
}
return response;
},
_onError(error) {
if (this.get('config.pushToStore') && isArray(error.errors)) {
let id = this.get('model.id');
let typeClass = this.get('store').modelFor(this.get('modelName'));
error.serializedErrors = this.get('serializer').extractErrors(this.get('store'), typeClass, error, id);
}
RSVP.rethrow(error);
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the split in three methods. :)
Thanks for teaching me how to use rethrow.
addon/actions/action.js
Outdated
@@ -94,6 +94,13 @@ export default Object.extend({ | |||
} else { | |||
return response; | |||
} | |||
}).catch((errors) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _promise
method is now too long :/ We should consider splitting responsibility for then
and catch
to separate methods eg _processResponse(response)
and _processErrors(errors)
tests/unit/models/post-test.js
Outdated
model.set('id', 1); | ||
assert.equal(store.peekAll('post').get('length'), 1); | ||
|
||
model.publish(payload).then((response) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle these tests in catch
:
model.publish(payload).catch((response) => {
assert.equal(response.base.length, 1);
assert.equal(response.base[0], 'First error');
assert.equal(store.peekAll('post').get('length'), 1);
done();
});
and to be honest, we should receive an ErrorObject
as the response
not serialized errors.
We should refer to them through model.get('errors')
... It won't be so easy to do that, but I think this will be a correct way, a compatible solution with the current ember-data flow.
Of course, this comment concern all tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget abut this idea :/
We should refer to them through model.get('errors')... It won't be so easy to do that, but I think this will be a correct way, a compatible solution with the current ember-data flow.
This can't fit into custom actions concept :/ Custom actions should be stateless so we shouldn't kept their errors in the model object :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think I missed something. I don't understand why custom actions should remain stateless. Indeed, this is not how ember-data behaves, correct me if I'm wrong ? In your opinion, how should we handle 422 errors sent by the server ? In ember-data, they are put in model.get('errors')
. Should we not do the same ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model.get('errors')
is used only if there are errors during removing, updating or saving a state of the model. So, there can be two reasons of that. First, model validation and second undefined errors.
If you are calling custom action you didn't change the state of the object. You are not saving it, removing or updating, so you are not changing his state. Of course, you can send an action which will return the model with new attributes, but it's not expected by custom action and you are not sending a whole model for that, just a few parameters.
If we will allow on assigning errors to the model we won't be able to control the lifecycle of them. When should they be cleared? After updating the record? Or after the recall? We don't even have an API for that :/
That's why I think, we should stay stateless without interacting with the model through custom actions.
So, as I just suggest higher, in my opinion, we should just add serializedErrors
to the error object to allow you to handle that in your catch
callback, eg:
model.publish({ publisherId: 1 }).catch((error) => {
this.set('publishErrors', error.serializedErrors);
});
Maybe in the future, when Ember will add public API for adding errors we will look at this topic again.
Hi @dcyriller, |
Hi @Exelord, thanks for your answers. They brings clarifications. :) |
addon/actions/action.js
Outdated
let model = this.get('model'); | ||
let store = this.get('store'); | ||
let errors = this.get('serializer').extractErrors(store, model.constructor, reason); | ||
store.recordWasInvalid(model._internalModel, errors); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about the same but unfortunately it's PRIVATE :/
@method recordWasInvalid
@private
@param {InternalModel} internalModel
@param {Object} errors
*/
recordWasInvalid(internalModel, errors) {
internalModel.adapterDidInvalidate(errors);
},
And as I was saying above, we can't assign errors to model in custom action because it will break an ember-data flow according to errors handling.
Let's imagine that we have a model Post and a custom action Publish. We are calling post.publish() and it will return an error eg "Publish action require passing publisher". Then we decided to update a title of the post. And unfortunately, after successful save our errors disappeared :/ Which should not be the case because we didn't publish successfully the post.
That's way they should stay separated from a model.
Some work here!