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

Breaks circular references in rejected jqXhr promises #12262

Merged
merged 1 commit into from
Sep 2, 2015

Conversation

xcambar
Copy link
Contributor

@xcambar xcambar commented Sep 1, 2015

Ember.RSVP.onerrorDefault creates a circular reference to jqXhr instances by ultimately doing:

e.errorThrown.__reason_with_error_thrown__ = e;

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

@xcambar
Copy link
Contributor Author

xcambar commented Sep 1, 2015

From travis, job Node.js: iojs — TEST_SUITE=old-jquery-and-extend-prototypes:

$ nvm install iojs
Installing iojs from source is not currently supported
The command "nvm install iojs" failed and exited with 105 during .

@rwjblue
Copy link
Member

rwjblue commented Sep 1, 2015

Installing iojs from source is not currently supported

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 JSON.stringify?

@stefanpenner
Copy link
Member

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;
Copy link
Member

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 })

@xcambar
Copy link
Contributor Author

xcambar commented Sep 1, 2015

@rwjblue Done. For readibility, I added an assertion on error.message because a failure in JSON.stringify recalls Ember.onerror with the TypeError: circular reference and a serialization to {}.
It is not necessary for the test itself, but helps in case of failure.

@stefanpenner Absolutely, I was not happy with the delete but simply did not think of defineProperty. Thanks and Done.

@xcambar xcambar force-pushed the rsvp_circular_reference branch 2 times, most recently from a2c2a0d to 5b909fa Compare September 1, 2015 21:11
@rwjblue
Copy link
Member

rwjblue commented Sep 2, 2015

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__', {
Copy link
Member

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.

@xcambar xcambar force-pushed the rsvp_circular_reference branch from 5b909fa to 4f6bad2 Compare September 2, 2015 05:36
@xcambar
Copy link
Contributor Author

xcambar commented Sep 2, 2015

@stefanpenner Fixed.

rwjblue added a commit that referenced this pull request Sep 2, 2015
Breaks circular references in rejected jqXhr promises
@rwjblue rwjblue merged commit e762025 into emberjs:master Sep 2, 2015
@rwjblue
Copy link
Member

rwjblue commented Sep 2, 2015

Awesome, thank you!

@xcambar xcambar deleted the rsvp_circular_reference branch September 2, 2015 11:58
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.

3 participants