provide isError and use it in place of instanceof Error #15872

graingert · 2017-03-30T08:54:57Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature/bug

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Angular should pass them on to the correct excetion handler, and there's an angular.isError function now.

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

re-implimented https://github.com/yefremov/iserror/blob/master/index.js to avoid license and make code faster.

Fixes #15868

graingert · 2017-03-30T14:56:39Z

@gkalpak what would be a good way to construct a test that boots up an iframe? And where should I put it?

function testError(createError) {
    var iframe = document.createElement('iframe');
    document.body.appendChild(iframe);
    try {
        var error = createError(contentWindow);
        expect(error instanceof Error).to.be.false();
        expect(isError(error)).to.be.true();
    } finally {
        iframe.parentElement.removeChild(iframe);
    }
}

testError(function (win) {
    return new win.Error();
});

testError(function (win) {
    try {
        win.document.querySelectorAll('');
    catch(e) {
        return e;
    }
});

gkalpak

A couple of minor comments. Looks OK otherwise (except for the missing tests).
The tests can go in https://github.com/graingert/angular.js/blob/af80f4765e2a9e795f64103b6e15770cf6eda050/test/AngularSpec.js.

gkalpak · 2017-03-30T14:53:14Z

src/Angular.js

@@ -675,6 +676,28 @@ var isArray = Array.isArray;

 /**
 * @ngdoc function
+ * @name angular.isError


I don't think we want to make this public.

gkalpak · 2017-03-30T14:55:35Z

src/Angular.js

+ * @returns {boolean} True if `value` is an `Error`.
+ */
+function isError(value) {
+  var tag = ({}).toString.call(value);


We already have a closure-global toString = Object.prototype.toString. You can simply toString.call(value).

gkalpak · 2017-03-30T14:56:48Z

test/minErrSpec.js

@@ -17,6 +17,7 @@ describe('minErr', function() {
  it('should return an Error factory', function() {
    var myError = testError('test', 'Oops');
    expect(myError instanceof Error).toBe(true);
+    expect(isError(myError)).toBe(true);


This seems irrelevant here. Please remove.

graingert · 2017-03-30T15:03:59Z

What's wrong with publishing it?

…

On 30 Mar 2017 16:02, "George Kalpakas" ***@***.***> wrote: ***@***.**** commented on this pull request. A couple of minor comments. Looks OK otherwise (except for the missing tests). The tests can go in https://github.com/graingert/angular.js/blob/ af80f47/test/AngularSpec.js. ------------------------------ In src/Angular.js <#15872 (comment)>: > @@ -675,6 +676,28 @@ var isArray = Array.isArray; /** * @ngdoc function + * @name angular.isError I don't think we want to make this public. ------------------------------ In src/Angular.js <#15872 (comment)>: > @@ -675,6 +676,28 @@ var isArray = Array.isArray; /** * @ngdoc function + * @name angular.isError + * @module ng + * @kind function + * + * @description + * Determines if a reference is an `Error`. + * + * @param {*} value Reference to check. + * @returns {boolean} True if `value` is an `Error`. + */ +function isError(value) { + var tag = ({}).toString.call(value); We already have a closure-global toString = Object.prototype.toString. You can simply toString.call(value). ------------------------------ In test/minErrSpec.js <#15872 (comment)>: > @@ -17,6 +17,7 @@ describe('minErr', function() { it('should return an Error factory', function() { var myError = testError('test', 'Oops'); expect(myError instanceof Error).toBe(true); + expect(isError(myError)).toBe(true); This seems irrelevant here. Please remove. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15872 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTITCKX7C5KczFgrrgsjIyNF_59gJks5rq8PvgaJpZM4MuDhG> .

gkalpak · 2017-03-30T15:11:44Z

What's wrong with publishing it?

We have to maintain it in a backwards compatible way. If it is not public we can change/improve/remove it without any concerns.

graingert · 2017-03-30T15:22:14Z

How do you not publish it?

…

On 30 Mar 2017 16:12, "George Kalpakas" ***@***.***> wrote: What's wrong with publishing it? We have to maintain it in a backwards compatible way. If it is not public we can change/improve/remove it without any concerns. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15872 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZQTBBUqkJGm3d_0jwZXROJIntlmhH7ks5rq8Z6gaJpZM4MuDhG> .

gkalpak · 2017-03-30T15:54:34Z

How do you not publish it?

Remove the @ngdoc anotation (and others that are not relevant/necessary: @name, @module ng, @kind).
(It won't be available anyway, so we don't want to have it appear on the docs either.)

graingert · 2017-03-30T16:03:36Z

src/ngMock/angular-mocks.js

@@ -984,7 +984,7 @@ angular.mock.dump = function(object) {
    } else if (angular.isObject(object)) {
      if (angular.isFunction(object.$eval) && angular.isFunction(object.$apply)) {
        out = serializeScope(object);
-      } else if (object instanceof Error) {
+      } else if (angular.isError(object)) {


a published isError is required here.

We could make it "private API" (e.g. angular.$$isError), but I don't think it is worth it. This function (angular.mock.dump) is very rarely used. It is also very rare to have new window contexts in unit tests and it is even more rare to have them throw errors. Plus, DOMExceptions get decently stringified by JSON.stringify (in contrast to plain Errors which become "{}"). All that considered, I think this one occurrence can remain object instanceof Error 😃

gkalpak · 2017-03-31T07:20:15Z

test/AngularSpec.js

+      expect(isError(new Error())).toBe(true);
+    });
+
+    function testError(createError) {


Move this either at the top or at the bottom of the describe block (or even create a nested describe for "other-context" tests) and give it a more descriptive name (e.g. testErrorFromDifferent context.

Maybe having a separate describe block with beforeAll/afterAll blocks for creating/destorying the iframe is a good idea.

try { } finally { } is more fine grained in this usecase

What do you mean?

it's specific to the actual call to the testErrorFromDifferentContext function. Like the lifetime of the iframe is shorter than a test, no point leaving it hanging around until afterAll/beforeAll is called.

I am fine either way, so feel free to leave things as they are 😃

gkalpak · 2017-03-31T07:31:20Z

src/ngMock/angular-mocks.js

@@ -984,7 +984,7 @@ angular.mock.dump = function(object) {
    } else if (angular.isObject(object)) {
      if (angular.isFunction(object.$eval) && angular.isFunction(object.$apply)) {
        out = serializeScope(object);
-      } else if (object instanceof Error) {
+      } else if (angular.isError(object)) {


We could make it "private API" (e.g. angular.$$isError), but I don't think it is worth it. This function (angular.mock.dump) is very rarely used. It is also very rare to have new window contexts in unit tests and it is even more rare to have them throw errors. Plus, DOMExceptions get decently stringified by JSON.stringify (in contrast to plain Errors which become "{}"). All that considered, I think this one occurrence can remain object instanceof Error 😃

graingert · 2017-04-03T10:19:31Z

@gkalpak ok, I'm pretty sure I've resolved everything. Interestingly enough if seems as though Safari thinks DOMExceptions from frames are instances of Errors

gkalpak

LGTM

gkalpak · 2017-04-27T12:48:16Z

test/AngularSpec.js

+
+    it('should not assume objects are errors', function() {
+      expect(isError({ message: 'A fake error', stack: 'no stack here'}))
+        .toBe(false);


Nit: Unnecessarily wrapped line.

gkalpak · 2017-04-27T12:54:21Z

(The commit message needs some love (as it doesn't follow our guidelines).)

gkalpak · 2017-04-27T13:17:02Z

I am going to merge this as is, but feel free to add DOMError in another PR 😁

graingert · 2017-04-27T13:26:06Z

~~I'd like to add DOMError too~~

scratch that, DOMError only has a 'name' property and never has a stack, so it shouldn't be added imhop.

gkalpak · 2017-04-27T14:13:05Z

Actually, before I merge it can you take care of the following:

Fix provide isError and use it in place of instanceof Error #15872 (comment).
Fix the commit message to follow the commit message guidelines.
Add a comment above the function pointing to the original project.

graingert · 2017-04-27T14:18:34Z

Fix the commit message to follow the commit message guidelines.

what message should I use?

gkalpak · 2017-04-27T15:11:01Z

Something that follows the guidelines. E.g. something like:

fix(*): correctly detect Error instances from different contexts

Errors thrown from different contexts (such as an iframe or webworker) are not detected as Error
instances and handled accordingly.
This commit fixes it by introducing an isError() helper, that is able to correctly detect such
instances.

Fixes #15868

12345678901234567890123456789012345678901234567890123456789012345678901234567890 Errors thrown from different contexts (such as an iframe or webworker) are not detected as Error instances and handled accordingly. This commit fixes it by introducing an isError() helper, that is able to correctly detect such instances. Fixes angular#15868

graingert · 2017-05-22T10:30:39Z

@gkalpak any updates on this?

Previously, errors thrown from different contexts (such as an iframe or webworker) were not detected as `Error` instances and handled accordingly. This commit fixes it by introducing an `isError()` helper, that is able to correctly detect such instances. Fixes #15868 Closes #15872

googlebot added the cla: yes label Mar 30, 2017

gkalpak mentioned this pull request Mar 30, 2017

provide isError and use it in place of instanceof Error #15868

Closed

3 tasks

gkalpak reviewed Mar 30, 2017

View reviewed changes

graingert commented Mar 30, 2017

View reviewed changes

graingert force-pushed the add-is-error branch 5 times, most recently from fdaccde to fb8f226 Compare March 30, 2017 20:31

gkalpak suggested changes Mar 31, 2017

View reviewed changes

graingert force-pushed the add-is-error branch 4 times, most recently from 85edd0f to a5710b1 Compare April 3, 2017 10:18

Narretz added the type: refactor label Apr 5, 2017

Narretz added this to the Backlog milestone Apr 5, 2017

graingert force-pushed the add-is-error branch from a5710b1 to 5bc7ce7 Compare April 18, 2017 08:24

gkalpak approved these changes Apr 27, 2017

View reviewed changes

petebacondarwin approved these changes Apr 27, 2017

View reviewed changes

graingert force-pushed the add-is-error branch from 5bc7ce7 to db0d876 Compare April 27, 2017 14:22

graingert force-pushed the add-is-error branch from db0d876 to a435740 Compare April 27, 2017 15:14

gkalpak closed this in 837acd1 May 22, 2017

peterdanis mentioned this pull request Feb 28, 2018

[WIP] Make toThrow matcher pass only if Error-like object is returned from promises jestjs/jest#5670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provide isError and use it in place of instanceof Error #15872

provide isError and use it in place of instanceof Error #15872

graingert commented Mar 30, 2017 •

edited

Loading

graingert commented Mar 30, 2017

gkalpak left a comment

gkalpak Mar 30, 2017

gkalpak Mar 30, 2017

gkalpak Mar 30, 2017

graingert commented Mar 30, 2017 via email

gkalpak commented Mar 30, 2017

graingert commented Mar 30, 2017 via email

gkalpak commented Mar 30, 2017

graingert Mar 30, 2017 •

edited

Loading

gkalpak Mar 31, 2017

gkalpak Mar 31, 2017

graingert Mar 31, 2017

gkalpak Mar 31, 2017

graingert Mar 31, 2017

gkalpak Apr 1, 2017

graingert Apr 3, 2017

gkalpak Mar 31, 2017

graingert commented Apr 3, 2017 •

edited

Loading

gkalpak left a comment

gkalpak Apr 27, 2017

gkalpak commented Apr 27, 2017

gkalpak commented Apr 27, 2017

graingert commented Apr 27, 2017

gkalpak commented Apr 27, 2017

graingert commented Apr 27, 2017

gkalpak commented Apr 27, 2017 •

edited

Loading

graingert commented May 22, 2017

provide isError and use it in place of instanceof Error #15872

provide isError and use it in place of instanceof Error #15872

Conversation

graingert commented Mar 30, 2017 • edited Loading

graingert commented Mar 30, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Mar 30, 2017 via email

gkalpak commented Mar 30, 2017

graingert commented Mar 30, 2017 via email

gkalpak commented Mar 30, 2017

graingert Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graingert commented Apr 3, 2017 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Apr 27, 2017

gkalpak commented Apr 27, 2017

graingert commented Apr 27, 2017

gkalpak commented Apr 27, 2017

graingert commented Apr 27, 2017

gkalpak commented Apr 27, 2017 • edited Loading

graingert commented May 22, 2017

graingert commented Mar 30, 2017 •

edited

Loading

graingert Mar 30, 2017 •

edited

Loading

graingert commented Apr 3, 2017 •

edited

Loading

gkalpak commented Apr 27, 2017 •

edited

Loading