fix: #595, refactor ZoneAwareError property copy (#597)

Author: JiaLiPassion

Diff for: lib/zone.ts

@@ -1330,38 +1330,118 @@ const Zone: ZoneType = (function(global: any) {
   let frameParserStrategy = null;
   const stackRewrite = 'stackRewrite';
 
-  const assignAll = function(to, from) {
-    if (!to) {
-      return to;
+  // fix #595, create property descriptor
+  // for error properties
+  const createProperty = function(props, key) {
+    // if property is already defined, skip it.
+    if (props[key]) {
+      return;
     }
+    // define a local property
+    // in case error property is not settable
+    const name = __symbol__(key);
+    props[key] = {
+      configurable: true,
+      enumerable: true,
+      get: function() {
+        // if local property has no value
+        // use internal error's property value
+        if (!this[name]) {
+          const error = this[__symbol__('error')];
+          if (error) {
+            this[name] = error[key];
+          }
+        }
+        return this[name];
+      },
+      set: function(value) {
+        // setter will set value to local property value
+        this[name] = value;
+      }
+    };
+  };
 
-    if (from) {
-      let keys = Object.getOwnPropertyNames(from);
-      for (let i = 0; i < keys.length; i++) {
-        const key = keys[i];
-        // Avoid bugs when hasOwnProperty is shadowed
-        if (Object.prototype.hasOwnProperty.call(from, key)) {
-          to[key] = from[key];
+  // fix #595, create property descriptor
+  // for error method properties
+  const createMethodProperty = function(props, key) {
+    if (props[key]) {
+      return;
+    }
+    props[key] = {
+      configurable: true,
+      enumerable: true,
+      writable: true,
+      value: function() {
+        const error = this[__symbol__('error')];
+        let errorMethod = (error && error[key]) || this[key];
+        if (errorMethod) {
+          return errorMethod.apply(error, arguments);
         }
       }
+    };
+  };
 
-      // copy all properties from prototype
-      // in Error, property such as name/message is in Error's prototype
-      // but not enumerable, so we copy those properties through
-      // Error's prototype
-      const proto = Object.getPrototypeOf(from);
-      if (proto) {
-        let pKeys = Object.getOwnPropertyNames(proto);
-        for (let i = 0; i < pKeys.length; i++) {
-          const key = pKeys[i];
-          // skip constructor
-          if (key !== 'constructor') {
-            to[key] = from[key];
-          }
+  const createErrorProperties = function() {
+    const props = Object.create(null);
+
+    const error = new NativeError();
+    let keys = Object.getOwnPropertyNames(error);
+    for (let i = 0; i < keys.length; i++) {
+      const key = keys[i];
+      // Avoid bugs when hasOwnProperty is shadowed
+      if (Object.prototype.hasOwnProperty.call(error, key)) {
+        createProperty(props, key);
+      }
+    }
+
+    const proto = NativeError.prototype;
+    if (proto) {
+      let pKeys = Object.getOwnPropertyNames(proto);
+      for (let i = 0; i < pKeys.length; i++) {
+        const key = pKeys[i];
+        // skip constructor
+        if (key !== 'constructor' && key !== 'toString' && key !== 'toSource') {
+          createProperty(props, key);
         }
       }
     }
-    return to;
+
+    // some other properties are not
+    // in NativeError
+    createProperty(props, 'originalStack');
+    createProperty(props, 'zoneAwareStack');
+
+    // define toString, toSource as method property
+    createMethodProperty(props, 'toString');
+    createMethodProperty(props, 'toSource');
+    return props;
+  };
+
+  const errorProperties = createErrorProperties();
+
+  // for derived Error class which extends ZoneAwareError
+  // we should not override the derived class's property
+  // so we create a new props object only copy the properties
+  // from errorProperties which not exist in derived Error's prototype
+  const getErrorPropertiesForPrototype = function(prototype) {
+    // if the prototype is ZoneAwareError.prototype
+    // we just return the prebuilt errorProperties.
+    if (prototype === ZoneAwareError.prototype) {
+      return errorProperties;
+    }
+    const newProps = Object.create(null);
+    const cKeys = Object.getOwnPropertyNames(errorProperties);
+    const keys = Object.getOwnPropertyNames(prototype);
+    cKeys.forEach(cKey => {
+      if (keys.filter(key => {
+                return key === cKey;
+              })
+              .length === 0) {
+        newProps[cKey] = errorProperties[cKey];
+      }
+    });
+
+    return newProps;
   };
 
   /**
@@ -1378,6 +1458,7 @@ const Zone: ZoneType = (function(global: any) {
     }
     // Create an Error.
     let error: Error = NativeError.apply(this, arguments);
+    this[__symbol__('error')] = error;
 
     // Save original stack trace
     error.originalStack = error.stack;
@@ -1414,7 +1495,10 @@ const Zone: ZoneType = (function(global: any) {
       }
       error.stack = error.zoneAwareStack = frames.join('\n');
     }
-    return assignAll(this, error);
+    // use defineProperties here instead of copy property value
+    // because of issue #595 which will break angular2.
+    Object.defineProperties(this, getErrorPropertiesForPrototype(Object.getPrototypeOf(this)));
+    return this;
   }
 
   // Copy the prototype so that instanceof operator works as expected

Diff for: test/common/Error.spec.ts

@@ -6,6 +6,75 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+// simulate @angular/facade/src/error.ts
+class BaseError extends Error {
+  /** @internal **/
+  _nativeError: Error;
+
+  constructor(message: string) {
+    super(message);
+    const nativeError = new Error(message) as any as Error;
+    this._nativeError = nativeError;
+  }
+
+  get message() {
+    return this._nativeError.message;
+  }
+  set message(message) {
+    this._nativeError.message = message;
+  }
+  get name() {
+    return this._nativeError.name;
+  }
+  get stack() {
+    return (this._nativeError as any).stack;
+  }
+  set stack(value) {
+    (this._nativeError as any).stack = value;
+  }
+  toString() {
+    return this._nativeError.toString();
+  }
+}
+
+class WrappedError extends BaseError {
+  originalError: any;
+
+  constructor(message: string, error: any) {
+    super(`${message} caused by: ${error instanceof Error ? error.message : error}`);
+    this.originalError = error;
+  }
+
+  get stack() {
+    return ((this.originalError instanceof Error ? this.originalError : this._nativeError) as any)
+        .stack;
+  }
+}
+
+class TestError extends WrappedError {
+  constructor(message: string, error: any) {
+    super(`${message} caused by: ${error instanceof Error ? error.message : error}`, error);
+  }
+
+  get message() {
+    return 'test ' + this.originalError.message;
+  }
+}
+
+class TestMessageError extends WrappedError {
+  constructor(message: string, error: any) {
+    super(`${message} caused by: ${error instanceof Error ? error.message : error}`, error);
+  }
+
+  get message() {
+    return 'test ' + this.originalError.message;
+  }
+
+  set message(value) {
+    this.originalError.message = value;
+  }
+}
+
 describe('ZoneAwareError', () => {
   // If the environment does not supports stack rewrites, then these tests will fail
   // and there is no point in running them.
@@ -67,6 +136,27 @@ describe('ZoneAwareError', () => {
     }
   });
 
+  it('should not use child Error class get/set in ZoneAwareError constructor', () => {
+    const func = () => {
+      const error = new BaseError('test');
+      expect(error.message).toEqual('test');
+    };
+
+    expect(func).not.toThrow();
+  });
+
+  it('should behave correctly with wrapped error', () => {
+    const error = new TestError('originalMessage', new Error('error message'));
+    expect(error.message).toEqual('test error message');
+    error.originalError.message = 'new error message';
+    expect(error.message).toEqual('test new error message');
+
+    const error1 = new TestMessageError('originalMessage', new Error('error message'));
+    expect(error1.message).toEqual('test error message');
+    error1.message = 'new error message';
+    expect(error1.message).toEqual('test new error message');
+  });
+
   it('should show zone names in stack frames and remove extra frames', () => {
     const rootZone = getRootZone();
     const innerZone = rootZone.fork({name: 'InnerZone'});