fix(zone.js): correct defineProperties for symbol props. #45060
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
AndrewKushnir
added
action: review
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? <!-- Please check the one that applies to this PR using "x". --> - [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: <!-- Please describe the current behavior that you are modifying, or link to a relevant issue. --> Issue Number: N/A Allows usage of Object.defineProperties with symbol properties. - [ ] Yes - [X] No <!-- If this PR contains a breaking change, please describe the impact and migration path for existing applications below. --> Not unless there is an application currently depending on a symbol property _not_ being applied by defineProperties.
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.
Update type definition to remove `any` cast
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
Contributor
JiaLiPassion
approved these changes
Feb 12, 2022
JiaLiPassion
added
target: minor
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.
Update type definition to remove `any` cast
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
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.
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.
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.
JiaLiPassion
suggested changes
Feb 14, 2022
Contributor
There was a problem hiding this comment.
Hi, @varomodt
It seems there are several commits not belong to this PR, and also several commits seems not changing files, could you check it and rebase the PR?
Also could you please update the commit message to meet the guideline here https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit,
thank you.
Contributor
|
@varomodt , I checked your commits, it seems there are some |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, this
definePropertiespatch 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.PR Checklist
Please check if your PR fulfills the following requirements:
I'm unsure if this change requires a change to tests or docs. Please let me know if it does.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Allows usage of Object.defineProperties with symbol properties.
Does this PR introduce a breaking change?
Not unless there is an application currently depending on a symbol property not being applied by defineProperties.
Other information