[BUGFIX release] Fix Ember.Error inheritance #15522
Conversation
|
@chadhietala I tested some cases manually, which seemed to work. Could you give it a try as well? |
packages/ember-debug/lib/error.js
Outdated
|
|
||
| let error = Error.call(this, message); | ||
| this.stack = error.stack; |
There was a problem hiding this comment.
if super() is called is this still needed?
There was a problem hiding this comment.
I think so in transpiled land
stefanpenner
force-pushed
the
fix-error-ctor
branch
from
b6ee5e0
to
868cf19
Compare
stefanpenner
force-pushed
the
fix-error-ctor
branch
from
868cf19
to
ca954dd
Compare
| return new EmberError(message); | ||
| } | ||
| export default EmberError; | ||
| function EmberError(message, code) { |
There was a problem hiding this comment.
Dumb question, but why make this a function instead of using the fancy ES6 classes?
There was a problem hiding this comment.
EmberError unfortunately supports being invoked without using new. This ends up being a less hacky solution. We should deprecate that, so we can eventually switch over entirely though.
There was a problem hiding this comment.
Cool! Thank you for taking a moment to respond to my question.
|
@stefanpenner IE is not happy in saucelabs :\ |
| export default EmberError; | ||
| function EmberError(message, code) { | ||
| if (this instanceof EmberError) { | ||
| let error = Error.call(this, message, code); |
There was a problem hiding this comment.
FYI: In V8, using super() would give you the benefit of omitting all frames up to and including the EmberError constructor (which is what I believe Error.captureStackTrace was used for). ES6 Error subclasses are basically the better Error.captureStackTrace. Copying error properties below would also be unnecessary.
I'm not sure how other browsers behave though.
There was a problem hiding this comment.
We aren't actually using real super (getting transpiled) also we still support an unfortunate scenario calling the constructor without new. Which ES6 classes don't support. We will need to deprecate that first, then remove in the future.
There was a problem hiding this comment.
I'll open another PR that deprecated the call constructor behavior so by the best LTS we can drop
|
test failure seems like one of the spurious sauce labs issues... restarting that job |
|
@stefanpenner this is failing for real (restarted to get the screenshots).
|
|
closing this, as #17216 made it obsolete |
As pointed out by @schuay #15516 our error constructor essentially calls super twice. The reasons seems to have been an error in ExtendableBuiltin constructor, which did not return its
applycall. I can’t seem to find the issue ExtendBuiltin is working around. I suspect we can simply remove itcc @schuay