Skip to content

Commit b437d12

Browse files
JiaLiPassionAndrewKushnir
authored andcommitted
fix(zone.js): defineProperties should also set symbol props (#45098)
Close #44095 Fix `defineProperties` patch not set `symbol` props issue. Co-authored-by: varomodt<[email protected]> Co-authored-by: AndrewKushnir<[email protected]> PR Close #45098
1 parent 5883e05 commit b437d12

File tree

2 files changed

+122
-4
lines changed

2 files changed

+122
-4
lines changed

packages/zone.js/lib/browser/define-property.ts

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,27 @@ export function propertyPatch() {
3535
return _tryDefineProperty(obj, prop, desc, originalConfigurableFlag);
3636
};
3737

38-
Object.defineProperties = function(obj, props) {
38+
Object.defineProperties = function<T>(obj: T, props: PropertyDescriptorMap&ThisType<any>&{
39+
[s: symbol]: PropertyDescriptor;
40+
}): T {
3941
Object.keys(props).forEach(function(prop) {
4042
Object.defineProperty(obj, prop, props[prop]);
4143
});
44+
for (const sym of Object.getOwnPropertySymbols(props)) {
45+
const desc = Object.getOwnPropertyDescriptor(props, sym);
46+
// Since `Object.getOwnPropertySymbols` returns *all* symbols,
47+
// including non-enumberable ones, retrieve property descriptor and check
48+
// enumerability there. Proceed with the rewrite only when a property is
49+
// enumberable to make the logic consistent with the way regular
50+
// properties are retrieved (via `Object.keys`, which respects
51+
// `enumerable: false` flag). More information:
52+
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Enumerability_and_ownership_of_properties#retrieval
53+
if (desc?.enumerable) {
54+
Object.defineProperty(obj, sym, props[sym]);
55+
}
56+
}
4257
return obj;
43-
};
58+
}
4459

4560
Object.create = <any>function(proto: any, propertiesObject: any) {
4661
if (typeof propertiesObject === 'object' && !Object.isFrozen(propertiesObject)) {
@@ -92,8 +107,8 @@ function _tryDefineProperty(obj: any, prop: string, desc: any, originalConfigura
92107
return _defineProperty(obj, prop, desc);
93108
} catch (error) {
94109
if (desc.configurable) {
95-
// In case of errors, when the configurable flag was likely set by rewriteDescriptor(), let's
96-
// retry with the original flag value
110+
// In case of errors, when the configurable flag was likely set by rewriteDescriptor(),
111+
// let's retry with the original flag value
97112
if (typeof originalConfigurableFlag == 'undefined') {
98113
delete desc.configurable;
99114
} else {

packages/zone.js/test/browser/define-property.spec.ts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,106 @@ describe('defineProperty', function() {
3737
expect((obj as any).prop).toBeFalsy();
3838
});
3939
});
40+
41+
describe('defineProperties', () => {
42+
it('should set string props', () => {
43+
const obj: any = {};
44+
Object.defineProperties(obj, {
45+
'property1': {value: true, writable: true, enumerable: true},
46+
'property2': {value: 'Hello', writable: false, enumerable: true},
47+
'property3': {
48+
enumerable: true,
49+
get: () => {
50+
return obj.p3
51+
},
52+
set: (val: string) => obj.p3 = val
53+
},
54+
'property4': {enumerable: false, writable: true, value: 'hidden'}
55+
});
56+
expect(Object.keys(obj).sort()).toEqual(['property1', 'property2', 'property3']);
57+
expect(obj.property1).toBeTrue();
58+
expect(obj.property2).toEqual('Hello');
59+
expect(obj.property3).toBeUndefined();
60+
expect(obj.property4).toEqual('hidden');
61+
obj.property1 = false;
62+
expect(obj.property1).toBeFalse();
63+
expect(() => obj.property2 = 'new Hello').toThrow();
64+
obj.property3 = 'property3';
65+
expect(obj.property3).toEqual('property3');
66+
obj.property4 = 'property4';
67+
expect(obj.property4).toEqual('property4');
68+
});
69+
it('should set symbol props', () => {
70+
let a = Symbol();
71+
let b = Symbol();
72+
const obj: any = {};
73+
Object.defineProperties(obj, {
74+
[a]: {value: true, writable: true},
75+
[b]: {get: () => obj.b1, set: (val: string) => obj.b1 = val}
76+
});
77+
expect(Object.keys(obj)).toEqual([]);
78+
expect(obj[a]).toBeTrue();
79+
expect(obj[b]).toBeUndefined();
80+
obj[a] = false;
81+
expect(obj[a]).toBeFalse();
82+
obj[b] = 'b1';
83+
expect(obj[b]).toEqual('b1');
84+
});
85+
it('should set string and symbol props', () => {
86+
let a = Symbol();
87+
const obj: any = {};
88+
Object.defineProperties(obj, {
89+
[a]: {value: true, writable: true},
90+
'property1': {value: true, writable: true, enumerable: true},
91+
});
92+
expect(Object.keys(obj)).toEqual(['property1']);
93+
expect(obj.property1).toBeTrue();
94+
expect(obj[a]).toBeTrue();
95+
obj.property1 = false;
96+
expect(obj.property1).toBeFalse();
97+
obj[a] = false;
98+
expect(obj[a]).toBeFalse();
99+
});
100+
it('should only set enumerable symbol props', () => {
101+
let a = Symbol();
102+
// Create a props object contains 2 symbols properties.
103+
// 1. Symbol a, the value is a PropertyDescriptor and enumerable is true.
104+
// 2. builtin Symbol.hasInstance, the value is a PropertyDescriptor and enumerable is false.
105+
// When set the props to the Test class with Object.defineProperties, only the enumerable props
106+
// should be set, so Symbol.hasInstance should not be set, and `instanceof Test` should not
107+
// throw error.
108+
const props = {};
109+
Object.defineProperty(props, a, {
110+
value: {
111+
value: true,
112+
configurable: true,
113+
writable: true,
114+
enumerable: true,
115+
},
116+
configurable: true,
117+
writable: true,
118+
enumerable: true,
119+
});
120+
Object.defineProperty(props, Symbol.hasInstance, {
121+
value: {
122+
value: () => {
123+
throw new Error('Cannot perform instanceof checks');
124+
},
125+
configurable: false,
126+
writable: false,
127+
enumerable: false,
128+
},
129+
configurable: false,
130+
writable: false,
131+
enumerable: false,
132+
});
133+
class Test {};
134+
const obj = new Test();
135+
Object.defineProperties(Test, props);
136+
expect(Object.keys(obj)).toEqual([]);
137+
expect((Test as any)[a]).toBeTrue();
138+
(Test as any)[a] = false;
139+
expect((Test as any)[a]).toBeFalse();
140+
expect(obj instanceof Test).toBeTrue();
141+
});
142+
});

0 commit comments

Comments
 (0)