-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Breaks circular references in rejected jqXhr promises #12262
Conversation
From travis, job
|
Weird. Likely just a fluke. Can you add a test that does something like (pseudo code): Ember.onerror = function(error) {
assert.ok(JSON.stringify(error), 'can stringify the error');
};
var error = new Error('whatever');
var rejectReason = { errorThrow: error);
Ember.RSVP.reject(rejectReason); To ensure we can |
mutating objects from somewhere else isn't a good idea. Lets instead mark this property as non enumerable at the source. |
if (typeof error === 'string') { | ||
error = new Error(error); | ||
} | ||
error.__reason_with_error_thrown__ = e; | ||
delete reason.errorThrown; | ||
error.__reason__ = reason; |
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.
lets skip the delete, and just mark this property as non enumerable.
Object.defineProperty(error, '__reason__', { value: reason, enumerable: false })
@rwjblue Done. For readibility, I added an assertion on @stefanpenner Absolutely, I was not happy with the |
a2c2a0d
to
5b909fa
Compare
Looks good to me. @stefanpenner - r? |
if (typeof error === 'string') { | ||
error = new Error(error); | ||
} | ||
error.__reason_with_error_thrown__ = e; | ||
defineProperty(error, '__reason_with_error_thrown__', { |
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.
why not just Object.defineProperty
? Its a language feature now, since we only support > ES5.
Tests that errors can be serialized
5b909fa
to
4f6bad2
Compare
@stefanpenner Fixed. |
Breaks circular references in rejected jqXhr promises
Awesome, thank you! |
Ember.RSVP.onerrorDefault
creates a circular reference to jqXhr instances by ultimately doing:It prevents libraries like Sentry to properly handle errors as they usually
JSON.stringify
stuff and this fails (because circular reference).Here's an example JSBin : https://emberjs.jsbin.com/kubefos/edit?html,js,output