From 99e93bdb50c5422f105d23620605f191dddaa94a Mon Sep 17 00:00:00 2001 From: Rob S Date: Fri, 11 Feb 2022 16:56:09 -0800 Subject: [PATCH 01/10] Fix defineProperties for symbol props. Currently, this `defineProperties` patch does not work for symbol properties which will not be enumerated by Object.keys. This CL adds a second case for symbols, with a small feature guard for IE. --- packages/zone.js/lib/browser/define-property.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index f7161dc9119d..44fde95854e1 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -39,6 +39,11 @@ export function propertyPatch() { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); + Object.getOwnPropertySymbols && + Object.getOwnPropertySymbols(props).forEach((sym: symbol) => { + // tslint:disable-next-line:no-any + Object.defineProperty(obj, sym, (props as any)[sym]); + }); return obj; }; From eb89be899b6c5f17f218400bce2244e869fda44c Mon Sep 17 00:00:00 2001 From: Rob S Date: Fri, 11 Feb 2022 17:53:25 -0800 Subject: [PATCH 02/10] Update define-property.ts --- packages/zone.js/lib/browser/define-property.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index 44fde95854e1..00bb3e15f36a 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -40,10 +40,10 @@ export function propertyPatch() { Object.defineProperty(obj, prop, props[prop]); }); Object.getOwnPropertySymbols && - Object.getOwnPropertySymbols(props).forEach((sym: symbol) => { - // tslint:disable-next-line:no-any - Object.defineProperty(obj, sym, (props as any)[sym]); - }); + Object.getOwnPropertySymbols(props).forEach((sym: symbol) => { + // tslint:disable-next-line:no-any + Object.defineProperty(obj, sym, (props as any)[sym]); + }); return obj; }; From 0701dc3fcdbfd5c941c5d523b9c5bc8d28663395 Mon Sep 17 00:00:00 2001 From: Robert Sloan Date: Fri, 11 Feb 2022 18:26:50 -0800 Subject: [PATCH 03/10] fix(zone.js): correct defineProperties for symbol props. Currently, this `defineProperties` patch does not work for symbol properties which will not be enumerated by Object.keys. This CL adds a second case for symbols, with a small feature guard for IE. Please check if your PR fulfills the following requirements: - [X] The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit - [ ] Tests for the changes have been added (for bug fixes / features) - [ ] Docs have been added / updated (for bug fixes / features) I'm unsure if this change requires a change to tests or docs. Please let me know if it does. What kind of change does this PR introduce? - [X] Bugfix - [ ] Feature - [ ] Code style update (formatting, local variables) - [ ] Refactoring (no functional changes, no api changes) - [ ] Build related changes - [ ] CI related changes - [ ] Documentation content changes - [ ] angular.io application / infrastructure changes - [ ] Other... Please describe: Issue Number: N/A Allows usage of Object.defineProperties with symbol properties. - [ ] Yes - [X] No Not unless there is an application currently depending on a symbol property _not_ being applied by defineProperties. --- packages/zone.js/lib/browser/define-property.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index f7161dc9119d..afc5182687b4 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -39,6 +39,12 @@ export function propertyPatch() { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); + if (Object.getOwnPropertySymbols) { + for (const sym of Object.getOwnPropertySymbols(props)) { + // tslint:disable-next-line:no-any + Object.defineProperty(obj, sym, (props as any)[sym]); + } + } return obj; }; From c09d4d1f880d2a0dfa0e721c996c3e2f131b40cd Mon Sep 17 00:00:00 2001 From: Rob S Date: Fri, 11 Feb 2022 18:31:41 -0800 Subject: [PATCH 04/10] Update define-property.ts --- packages/zone.js/lib/browser/define-property.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index 00bb3e15f36a..afc5182687b4 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -39,11 +39,12 @@ export function propertyPatch() { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); - Object.getOwnPropertySymbols && - Object.getOwnPropertySymbols(props).forEach((sym: symbol) => { - // tslint:disable-next-line:no-any - Object.defineProperty(obj, sym, (props as any)[sym]); - }); + if (Object.getOwnPropertySymbols) { + for (const sym of Object.getOwnPropertySymbols(props)) { + // tslint:disable-next-line:no-any + Object.defineProperty(obj, sym, (props as any)[sym]); + } + } return obj; }; From 791942fef25085c4401006a715321554faf3cbb0 Mon Sep 17 00:00:00 2001 From: Rob S Date: Fri, 11 Feb 2022 18:31:41 -0800 Subject: [PATCH 05/10] fix(zone.js): correct defineProperties for symbol props. --- packages/zone.js/lib/browser/define-property.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index 00bb3e15f36a..afc5182687b4 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -39,11 +39,12 @@ export function propertyPatch() { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); - Object.getOwnPropertySymbols && - Object.getOwnPropertySymbols(props).forEach((sym: symbol) => { - // tslint:disable-next-line:no-any - Object.defineProperty(obj, sym, (props as any)[sym]); - }); + if (Object.getOwnPropertySymbols) { + for (const sym of Object.getOwnPropertySymbols(props)) { + // tslint:disable-next-line:no-any + Object.defineProperty(obj, sym, (props as any)[sym]); + } + } return obj; }; From 9af215186bf742ee123c50dcbddac84483c5d97d Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 12 Feb 2022 17:15:05 +0900 Subject: [PATCH 06/10] fix(zone.js): update type definition of defineProperties Update type definition to remove `any` cast --- packages/zone.js/lib/browser/define-property.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index afc5182687b4..6736e210c966 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -35,14 +35,15 @@ export function propertyPatch() { return _tryDefineProperty(obj, prop, desc, originalConfigurableFlag); }; - Object.defineProperties = function(obj, props) { + Object.defineProperties = function(obj: T, props: PropertyDescriptorMap&ThisType&{ + [s: symbol]: PropertyDescriptor; + }): T { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); if (Object.getOwnPropertySymbols) { for (const sym of Object.getOwnPropertySymbols(props)) { - // tslint:disable-next-line:no-any - Object.defineProperty(obj, sym, (props as any)[sym]); + Object.defineProperty(obj, sym, props[sym]); } } return obj; From c490725bc585b816ec65fee1d43a06115340d19b Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 12 Feb 2022 17:15:37 +0900 Subject: [PATCH 07/10] test(zone.js): add test cases of Object.defineProperties() Add test cases of `Object.definiProperties()` zone.js patch to test 1. defineProperties for normal string props 2. defineProperties for symbol props 3. defineProperties for normal string props + symbol props --- .../test/browser/define-property.spec.ts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/packages/zone.js/test/browser/define-property.spec.ts b/packages/zone.js/test/browser/define-property.spec.ts index 13818f72975a..2ca2531dcddf 100644 --- a/packages/zone.js/test/browser/define-property.spec.ts +++ b/packages/zone.js/test/browser/define-property.spec.ts @@ -37,3 +37,64 @@ describe('defineProperty', function() { expect((obj as any).prop).toBeFalsy(); }); }); + +describe('defineProperties', () => { + it('should set string props', () => { + const obj: any = {}; + Object.defineProperties(obj, { + 'property1': {value: true, writable: true, enumerable: true}, + 'property2': {value: 'Hello', writable: false, enumerable: true}, + 'property3': { + enumerable: true, + get: () => { + return obj.p3 + }, + set: (val: string) => obj.p3 = val + }, + 'property4': {enumerable: false, writable: true, value: 'hidden'} + }); + expect(Object.keys(obj).sort()).toEqual(['property1', 'property2', 'property3']); + expect(obj.property1).toBeTrue(); + expect(obj.property2).toEqual('Hello'); + expect(obj.property3).toBeUndefined(); + expect(obj.property4).toEqual('hidden'); + obj.property1 = false; + expect(obj.property1).toBeFalse(); + expect(() => obj.property2 = 'new Hello').toThrow(); + obj.property3 = 'property3'; + expect(obj.property3).toEqual('property3'); + obj.property4 = 'property4'; + expect(obj.property4).toEqual('property4'); + }); + it('should set symbol props', () => { + let a = Symbol(); + let b = Symbol(); + const obj: any = {}; + Object.defineProperties(obj, { + [a]: {value: true, writable: true}, + [b]: {get: () => obj.b1, set: (val: string) => obj.b1 = val} + }); + expect(Object.keys(obj)).toEqual([]); + expect(obj[a]).toBeTrue(); + expect(obj[b]).toBeUndefined(); + obj[a] = false; + expect(obj[a]).toBeFalse(); + obj[b] = 'b1'; + expect(obj[b]).toEqual('b1'); + }); + it('should set string and symbol props', () => { + let a = Symbol(); + const obj: any = {}; + Object.defineProperties(obj, { + [a]: {value: true, writable: true}, + 'property1': {value: true, writable: true, enumerable: true}, + }); + expect(Object.keys(obj)).toEqual(['property1']); + expect(obj.property1).toBeTrue(); + expect(obj[a]).toBeTrue(); + obj.property1 = false; + expect(obj.property1).toBeFalse(); + obj[a] = false; + expect(obj[a]).toBeFalse(); + }); +}); From b299f94c39545ff93a484707d605b854a523ed7c Mon Sep 17 00:00:00 2001 From: Rob S Date: Fri, 11 Feb 2022 16:56:09 -0800 Subject: [PATCH 08/10] fix(zone.js): correct defineProperties for symbol props. Currently, this `defineProperties` patch does not work for symbol properties which will not be enumerated by Object.keys. This CL adds a second case for symbols, with a small feature guard for IE. --- packages/zone.js/lib/browser/define-property.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index f7161dc9119d..afc5182687b4 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -39,6 +39,12 @@ export function propertyPatch() { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); + if (Object.getOwnPropertySymbols) { + for (const sym of Object.getOwnPropertySymbols(props)) { + // tslint:disable-next-line:no-any + Object.defineProperty(obj, sym, (props as any)[sym]); + } + } return obj; }; From 359352344451d84a2fb574f6f684493aed26276f Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 12 Feb 2022 17:15:05 +0900 Subject: [PATCH 09/10] fix(zone.js): update type definition of defineProperties Update type definition to remove `any` cast --- packages/zone.js/lib/browser/define-property.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/zone.js/lib/browser/define-property.ts b/packages/zone.js/lib/browser/define-property.ts index afc5182687b4..6736e210c966 100644 --- a/packages/zone.js/lib/browser/define-property.ts +++ b/packages/zone.js/lib/browser/define-property.ts @@ -35,14 +35,15 @@ export function propertyPatch() { return _tryDefineProperty(obj, prop, desc, originalConfigurableFlag); }; - Object.defineProperties = function(obj, props) { + Object.defineProperties = function(obj: T, props: PropertyDescriptorMap&ThisType&{ + [s: symbol]: PropertyDescriptor; + }): T { Object.keys(props).forEach(function(prop) { Object.defineProperty(obj, prop, props[prop]); }); if (Object.getOwnPropertySymbols) { for (const sym of Object.getOwnPropertySymbols(props)) { - // tslint:disable-next-line:no-any - Object.defineProperty(obj, sym, (props as any)[sym]); + Object.defineProperty(obj, sym, props[sym]); } } return obj; From 7728ef33db6b69bf2b477d6749eb6539e4e821fb Mon Sep 17 00:00:00 2001 From: JiaLiPassion Date: Sat, 12 Feb 2022 17:15:37 +0900 Subject: [PATCH 10/10] test(zone.js): add test cases of Object.defineProperties() Add test cases of `Object.definiProperties()` zone.js patch to test 1. defineProperties for normal string props 2. defineProperties for symbol props 3. defineProperties for normal string props + symbol props --- .../test/browser/define-property.spec.ts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/packages/zone.js/test/browser/define-property.spec.ts b/packages/zone.js/test/browser/define-property.spec.ts index 13818f72975a..2ca2531dcddf 100644 --- a/packages/zone.js/test/browser/define-property.spec.ts +++ b/packages/zone.js/test/browser/define-property.spec.ts @@ -37,3 +37,64 @@ describe('defineProperty', function() { expect((obj as any).prop).toBeFalsy(); }); }); + +describe('defineProperties', () => { + it('should set string props', () => { + const obj: any = {}; + Object.defineProperties(obj, { + 'property1': {value: true, writable: true, enumerable: true}, + 'property2': {value: 'Hello', writable: false, enumerable: true}, + 'property3': { + enumerable: true, + get: () => { + return obj.p3 + }, + set: (val: string) => obj.p3 = val + }, + 'property4': {enumerable: false, writable: true, value: 'hidden'} + }); + expect(Object.keys(obj).sort()).toEqual(['property1', 'property2', 'property3']); + expect(obj.property1).toBeTrue(); + expect(obj.property2).toEqual('Hello'); + expect(obj.property3).toBeUndefined(); + expect(obj.property4).toEqual('hidden'); + obj.property1 = false; + expect(obj.property1).toBeFalse(); + expect(() => obj.property2 = 'new Hello').toThrow(); + obj.property3 = 'property3'; + expect(obj.property3).toEqual('property3'); + obj.property4 = 'property4'; + expect(obj.property4).toEqual('property4'); + }); + it('should set symbol props', () => { + let a = Symbol(); + let b = Symbol(); + const obj: any = {}; + Object.defineProperties(obj, { + [a]: {value: true, writable: true}, + [b]: {get: () => obj.b1, set: (val: string) => obj.b1 = val} + }); + expect(Object.keys(obj)).toEqual([]); + expect(obj[a]).toBeTrue(); + expect(obj[b]).toBeUndefined(); + obj[a] = false; + expect(obj[a]).toBeFalse(); + obj[b] = 'b1'; + expect(obj[b]).toEqual('b1'); + }); + it('should set string and symbol props', () => { + let a = Symbol(); + const obj: any = {}; + Object.defineProperties(obj, { + [a]: {value: true, writable: true}, + 'property1': {value: true, writable: true, enumerable: true}, + }); + expect(Object.keys(obj)).toEqual(['property1']); + expect(obj.property1).toBeTrue(); + expect(obj[a]).toBeTrue(); + obj.property1 = false; + expect(obj.property1).toBeFalse(); + obj[a] = false; + expect(obj[a]).toBeFalse(); + }); +});