Conversation
| // Response | ||
| expect(responseParams).to.have.property('method').to.equal('POST'); | ||
| expect(responseParams).to.have.property('message').to.equal('conversion type must be one of add_to_wishlist, add_to_cart, like, message, make_offer, read. If you wish to use custom types, please set is_custom_type to true and specify a display_name.'); | ||
| expect(responseParams).to.have.property('message').to.equal('Invalid parameters'); |
There was a problem hiding this comment.
The error message has been moved to response.errors[x].message but we don't emit that in our RequestQueue at the moment on errors. I didn't want to change the implementation just because of this
There was a problem hiding this comment.
Do you know if this is how errors are going to be surfaced moving forward? If so, it shouldn't be too hard to add it as a part of the emitted event (and it should be backwards compatible since it's an addition). If it's something that's expected to continue changing, I wonder if we should just spread (...) the JSON body into the emitted event. For example:
instance.eventemitter.emit('error', {
url: nextInQueue.url,
method: nextInQueue.method,
...(json && {...json}) // bringing back of one of Stanley's signature existence-check and spread
});
There was a problem hiding this comment.
I like that. Should I create a story for it?
There was a problem hiding this comment.
@esezen Looks like a couple other tests are failing because we no longer validate item id when beacon=false for click events.
So we aren't returning an error anymore.
Changes here look good but I don't know if we want to wait for backend to finalize their changes, or include these changes in this PR.
Looks like Jimmy created another PR to fix the tests so NVM.
There was a problem hiding this comment.
Changes look good from what I can tell. Just left a minor comment/question.
Thanks for fixing them! ❤️
Looks like Jimmy created another PR to fix the tests so NVM.
I was not aware of this 😂 I think @stanlp1 might've meant story?
| // Response | ||
| expect(responseParams).to.have.property('method').to.equal('POST'); | ||
| expect(responseParams).to.have.property('message').to.equal('conversion type must be one of add_to_wishlist, add_to_cart, like, message, make_offer, read. If you wish to use custom types, please set is_custom_type to true and specify a display_name.'); | ||
| expect(responseParams).to.have.property('message').to.equal('Invalid parameters'); |
There was a problem hiding this comment.
Do you know if this is how errors are going to be surfaced moving forward? If so, it shouldn't be too hard to add it as a part of the emitted event (and it should be backwards compatible since it's an addition). If it's something that's expected to continue changing, I wonder if we should just spread (...) the JSON body into the emitted event. For example:
instance.eventemitter.emit('error', {
url: nextInQueue.url,
method: nextInQueue.method,
...(json && {...json}) // bringing back of one of Stanley's signature existence-check and spread
});
3 participants
No description provided.