Skip to content
This repository was archived by the owner on Feb 26, 2024. It is now read-only.

Commit f7330de

Browse files
JiaLiPassionmhevery
JiaLiPassion
authored and
mhevery
committed
fix: #595, refactor ZoneAwareError property copy (#597)
1 parent 83dfa97 commit f7330de

2 files changed

Lines changed: 199 additions & 25 deletions

File tree

‎lib/zone.ts‎

Lines changed: 109 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1330,38 +1330,118 @@ const Zone: ZoneType = (function(global: any) {
13301330
let frameParserStrategy = null;
13311331
const stackRewrite = 'stackRewrite';
13321332

1333-
const assignAll = function(to, from) {
1334-
if (!to) {
1335-
return to;
1333+
// fix #595, create property descriptor
1334+
// for error properties
1335+
const createProperty = function(props, key) {
1336+
// if property is already defined, skip it.
1337+
if (props[key]) {
1338+
return;
13361339
}
1340+
// define a local property
1341+
// in case error property is not settable
1342+
const name = __symbol__(key);
1343+
props[key] = {
1344+
configurable: true,
1345+
enumerable: true,
1346+
get: function() {
1347+
// if local property has no value
1348+
// use internal error's property value
1349+
if (!this[name]) {
1350+
const error = this[__symbol__('error')];
1351+
if (error) {
1352+
this[name] = error[key];
1353+
}
1354+
}
1355+
return this[name];
1356+
},
1357+
set: function(value) {
1358+
// setter will set value to local property value
1359+
this[name] = value;
1360+
}
1361+
};
1362+
};
13371363

1338-
if (from) {
1339-
let keys = Object.getOwnPropertyNames(from);
1340-
for (let i = 0; i < keys.length; i++) {
1341-
const key = keys[i];
1342-
// Avoid bugs when hasOwnProperty is shadowed
1343-
if (Object.prototype.hasOwnProperty.call(from, key)) {
1344-
to[key] = from[key];
1364+
// fix #595, create property descriptor
1365+
// for error method properties
1366+
const createMethodProperty = function(props, key) {
1367+
if (props[key]) {
1368+
return;
1369+
}
1370+
props[key] = {
1371+
configurable: true,
1372+
enumerable: true,
1373+
writable: true,
1374+
value: function() {
1375+
const error = this[__symbol__('error')];
1376+
let errorMethod = (error && error[key]) || this[key];
1377+
if (errorMethod) {
1378+
return errorMethod.apply(error, arguments);
13451379
}
13461380
}
1381+
};
1382+
};
13471383

1348-
// copy all properties from prototype
1349-
// in Error, property such as name/message is in Error's prototype
1350-
// but not enumerable, so we copy those properties through
1351-
// Error's prototype
1352-
const proto = Object.getPrototypeOf(from);
1353-
if (proto) {
1354-
let pKeys = Object.getOwnPropertyNames(proto);
1355-
for (let i = 0; i < pKeys.length; i++) {
1356-
const key = pKeys[i];
1357-
// skip constructor
1358-
if (key !== 'constructor') {
1359-
to[key] = from[key];
1360-
}
1384+
const createErrorProperties = function() {
1385+
const props = Object.create(null);
1386+
1387+
const error = new NativeError();
1388+
let keys = Object.getOwnPropertyNames(error);
1389+
for (let i = 0; i < keys.length; i++) {
1390+
const key = keys[i];
1391+
// Avoid bugs when hasOwnProperty is shadowed
1392+
if (Object.prototype.hasOwnProperty.call(error, key)) {
1393+
createProperty(props, key);
1394+
}
1395+
}
1396+
1397+
const proto = NativeError.prototype;
1398+
if (proto) {
1399+
let pKeys = Object.getOwnPropertyNames(proto);
1400+
for (let i = 0; i < pKeys.length; i++) {
1401+
const key = pKeys[i];
1402+
// skip constructor
1403+
if (key !== 'constructor' && key !== 'toString' && key !== 'toSource') {
1404+
createProperty(props, key);
13611405
}
13621406
}
13631407
}
1364-
return to;
1408+
1409+
// some other properties are not
1410+
// in NativeError
1411+
createProperty(props, 'originalStack');
1412+
createProperty(props, 'zoneAwareStack');
1413+
1414+
// define toString, toSource as method property
1415+
createMethodProperty(props, 'toString');
1416+
createMethodProperty(props, 'toSource');
1417+
return props;
1418+
};
1419+
1420+
const errorProperties = createErrorProperties();
1421+
1422+
// for derived Error class which extends ZoneAwareError
1423+
// we should not override the derived class's property
1424+
// so we create a new props object only copy the properties
1425+
// from errorProperties which not exist in derived Error's prototype
1426+
const getErrorPropertiesForPrototype = function(prototype) {
1427+
// if the prototype is ZoneAwareError.prototype
1428+
// we just return the prebuilt errorProperties.
1429+
if (prototype === ZoneAwareError.prototype) {
1430+
return errorProperties;
1431+
}
1432+
const newProps = Object.create(null);
1433+
const cKeys = Object.getOwnPropertyNames(errorProperties);
1434+
const keys = Object.getOwnPropertyNames(prototype);
1435+
cKeys.forEach(cKey => {
1436+
if (keys.filter(key => {
1437+
return key === cKey;
1438+
})
1439+
.length === 0) {
1440+
newProps[cKey] = errorProperties[cKey];
1441+
}
1442+
});
1443+
1444+
return newProps;
13651445
};
13661446

13671447
/**
@@ -1378,6 +1458,7 @@ const Zone: ZoneType = (function(global: any) {
13781458
}
13791459
// Create an Error.
13801460
let error: Error = NativeError.apply(this, arguments);
1461+
this[__symbol__('error')] = error;
13811462

13821463
// Save original stack trace
13831464
error.originalStack = error.stack;
@@ -1414,7 +1495,10 @@ const Zone: ZoneType = (function(global: any) {
14141495
}
14151496
error.stack = error.zoneAwareStack = frames.join('\n');
14161497
}
1417-
return assignAll(this, error);
1498+
// use defineProperties here instead of copy property value
1499+
// because of issue #595 which will break angular2.
1500+
Object.defineProperties(this, getErrorPropertiesForPrototype(Object.getPrototypeOf(this)));
1501+
return this;
14181502
}
14191503

14201504
// Copy the prototype so that instanceof operator works as expected

‎test/common/Error.spec.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,75 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
// simulate @angular/facade/src/error.ts
10+
class BaseError extends Error {
11+
/** @internal **/
12+
_nativeError: Error;
13+
14+
constructor(message: string) {
15+
super(message);
16+
const nativeError = new Error(message) as any as Error;
17+
this._nativeError = nativeError;
18+
}
19+
20+
get message() {
21+
return this._nativeError.message;
22+
}
23+
set message(message) {
24+
this._nativeError.message = message;
25+
}
26+
get name() {
27+
return this._nativeError.name;
28+
}
29+
get stack() {
30+
return (this._nativeError as any).stack;
31+
}
32+
set stack(value) {
33+
(this._nativeError as any).stack = value;
34+
}
35+
toString() {
36+
return this._nativeError.toString();
37+
}
38+
}
39+
40+
class WrappedError extends BaseError {
41+
originalError: any;
42+
43+
constructor(message: string, error: any) {
44+
super(`${message} caused by: ${error instanceof Error ? error.message : error}`);
45+
this.originalError = error;
46+
}
47+
48+
get stack() {
49+
return ((this.originalError instanceof Error ? this.originalError : this._nativeError) as any)
50+
.stack;
51+
}
52+
}
53+
54+
class TestError extends WrappedError {
55+
constructor(message: string, error: any) {
56+
super(`${message} caused by: ${error instanceof Error ? error.message : error}`, error);
57+
}
58+
59+
get message() {
60+
return 'test ' + this.originalError.message;
61+
}
62+
}
63+
64+
class TestMessageError extends WrappedError {
65+
constructor(message: string, error: any) {
66+
super(`${message} caused by: ${error instanceof Error ? error.message : error}`, error);
67+
}
68+
69+
get message() {
70+
return 'test ' + this.originalError.message;
71+
}
72+
73+
set message(value) {
74+
this.originalError.message = value;
75+
}
76+
}
77+
978
describe('ZoneAwareError', () => {
1079
// If the environment does not supports stack rewrites, then these tests will fail
1180
// and there is no point in running them.
@@ -67,6 +136,27 @@ describe('ZoneAwareError', () => {
67136
}
68137
});
69138

139+
it('should not use child Error class get/set in ZoneAwareError constructor', () => {
140+
const func = () => {
141+
const error = new BaseError('test');
142+
expect(error.message).toEqual('test');
143+
};
144+
145+
expect(func).not.toThrow();
146+
});
147+
148+
it('should behave correctly with wrapped error', () => {
149+
const error = new TestError('originalMessage', new Error('error message'));
150+
expect(error.message).toEqual('test error message');
151+
error.originalError.message = 'new error message';
152+
expect(error.message).toEqual('test new error message');
153+
154+
const error1 = new TestMessageError('originalMessage', new Error('error message'));
155+
expect(error1.message).toEqual('test error message');
156+
error1.message = 'new error message';
157+
expect(error1.message).toEqual('test new error message');
158+
});
159+
70160
it('should show zone names in stack frames and remove extra frames', () => {
71161
const rootZone = getRootZone();
72162
const innerZone = rootZone.fork({name: 'InnerZone'});

0 commit comments

Comments
 (0)