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

Use native Error instead of custom Error subclass. #17216

Merged
merged 1 commit into from Dec 15, 2018

Conversation

Projects
None yet
7 participants
@rwjblue
Copy link
Member

rwjblue commented Nov 19, 2018

We originally created a custom subclass of Error because we speculatively might want to be able to discern "Ember error" from "non-Ember errors". Over the years, this custom Error subclass has caused us a significant amount of pain and I do not think that we have actually leveraged the "custom error subclass" concept much at all. This removes the custom subclass and replaces it with Error...

@rwjblue rwjblue force-pushed the rwjblue:use-native-error branch from da88c32 to a5fb2f3 Nov 19, 2018

@krisselden

This comment has been minimized.

Copy link
Member

krisselden commented Nov 19, 2018

This maybe considered a breaking change if people are using instanceof to conditionally check

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Nov 19, 2018

Please note that just because a given change might be observable, does not mean that it actually is breaking...


The thing that you are concerned with is where you were trying to see if a caught error was an Ember.Error but didn't want to handle non-Ember errors, right?

I just don't think this is a valid "thing to do" or care about. I also cannot find code on emberobserver.com codesearch that would fail with this change:

Out of all of the cases I reviewed (including ember-qunit-assert-helpers which previously did only catch Ember.Error's), I don't believe any would actually break with this change...

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Nov 21, 2018

I added @stefanpenner as a reviewer also, I know he has had thoughts on this in the past...

@Turbo87

Turbo87 approved these changes Dec 4, 2018

Copy link
Member

Turbo87 left a comment

yes, please! 🙏

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Dec 4, 2018

This maybe considered a breaking change if people are using instanceof to conditionally check

^^ Is my only concern.

I think @rwjblue approach to investigating risk here is quite good though, so I would like to see if we can proceed.

Thoughts on:

  • @rwjblue have you been able to do an internal code search here at work? We have very large amounts of ember code, that stress tests everything. It may yield further evidence.
  • we could consider flag, to allow affected users to opt out if needed? (may not be needed).

In general, I am more inclined to rip the bandaid off if possible, be we should be cognizant of any fallout. I believe your diligence is approaching sufficient, at-least for me.

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Dec 5, 2018

@rwjblue I think this might be worth a short RFC, if just to document the tradeoffs and decision-making that went into it. If you don't have bandwidth for it, I bet we could find someone in the community to help us write it up.

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Dec 5, 2018

@tomdale - I disagree. This doesn't add a deprecation, add new public APIs, or change existing public APIs in a way that apps/addons will be affected.

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Dec 5, 2018

@rwjblue Fair enough — forgive me for not adequately reading your analysis above about looking for (and not finding) any examples of code that would break in the wild.

My concern is companies that may have custom instrumentation that integrates with proprietary error reporting infrastructure. Examples of that may be hard to track down as they are unlikely to be open source. In this case, instanceof checks changing from true to false are unlikely to produce any errors, just generate subtly wrong or misleading behavior.

Without litigating whether instanceof is part of the implicit API of Ember via the exceptions it throws (which I think a reasonable person could argue either way), I just want to make sure someone who upgrades and runs into a problem has a reasonable chance of knowing about this change before embarking on a multi-hour debugging session.

If you don't think an RFC is appropriate, would you be open to making sure we include a mention of it in the release notes/blog post?

@stefanpenner

This comment has been minimized.

Copy link
Member

stefanpenner commented Dec 5, 2018

If you don't think an RFC is appropriate, would you be open to making sure we include a mention of it in the release notes/blog post?

if @tomdale is good with it, I believe explicit callout of the change (and potential risk) is sufficient to move forward.

@tomdale

This comment has been minimized.

Copy link
Member

tomdale commented Dec 12, 2018

@stefanpenner Sounds good to me. 👍

@rwjblue rwjblue force-pushed the rwjblue:use-native-error branch from a5fb2f3 to 407ea95 Dec 15, 2018

@rwjblue

This comment has been minimized.

Copy link
Member Author

rwjblue commented Dec 15, 2018

Thanks for working through this y'all. I've just rebased, and will coordinate with @kategengler on the CHANGELOG entry.

@rwjblue rwjblue merged commit 3841aa1 into emberjs:master Dec 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the rwjblue:use-native-error branch Dec 15, 2018

@amk221

This comment has been minimized.

Copy link
Contributor

amk221 commented Mar 6, 2019

Was there a mention of this in a blog post as talked about above? It caught us out with instanceof being true locally, but false in prod. Breaking a few things.

@xg-wang

This comment has been minimized.

Copy link
Contributor

xg-wang commented Mar 6, 2019

The advantage of ember's own Error class is it's easier to extend with babel transpilation, especially babel6. Otherwise, users need some custom hack to make the extending work with stack and instanceof check.

See details on MDN and StackOverflow.

@amk221

This comment has been minimized.

Copy link
Contributor

amk221 commented Mar 6, 2019

@xg-wang Was that a reply to me? It's not what I asked that's all.

@xg-wang

This comment has been minimized.

Copy link
Contributor

xg-wang commented Mar 6, 2019

@amk221 I haven't done any tests, I just ran into the MDN doc and have concerns about changes in this PR. But based on the babel issue described I think your issue about instanceof check may relate to it.

@amk221

This comment has been minimized.

Copy link
Contributor

amk221 commented Mar 6, 2019

@xg-wang My issue is that this broke our site as we didn't know about the change. (The fix was simply to not extend EmberError)

@xg-wang

This comment has been minimized.

Copy link
Contributor

xg-wang commented Mar 6, 2019

@amk221 yes it has issue with instanceof check right? MDN says both babel 6 and 7 babel 7 cannot handle extend and instanceof check without proper configuration or app code. But the old EmberError plays nice with it.

@Turbo87

This comment has been minimized.

Copy link
Member

Turbo87 commented Mar 6, 2019

@xg-wang Babel 7 seems to work quite well with it, as long as it is able to detect that you're extending the native Error class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.