Skip to content

getTextOfPropertyName: Assert input value is a PropertyName#21981

Merged
1 commit merged into
masterfrom
getTextOfPropertyName
Feb 16, 2018
Merged

getTextOfPropertyName: Assert input value is a PropertyName#21981
1 commit merged into
masterfrom
getTextOfPropertyName

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Feb 15, 2018

...which detects an invalid cast.

@ghost ghost force-pushed the getTextOfPropertyName branch from 57bc1a2 to a3c7fd1 Compare February 15, 2018 22:55
@ghost ghost requested a review from weswigham February 15, 2018 23:13
Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the assertion addition and the removal of casts. Just the comment below, NBD if one can't be found, though.

Comment thread src/compiler/checker.ts
if (parent.initializer && property) {
checkPropertyAccessibility(parent, parent.initializer, parentType, property);
const name = node.propertyName || node.name;
if (!isBindingPattern(name)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a documented bug this change fixes? If not, can you come up with a test case which would have crashed/error'd/behaved differently before? It'd be nice to just the coverage if possible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously getTextOfPropertyName would have just returned undefined, then so would getPropertyOfType and it would be ignored. So there's no observable difference in behavior.

@ghost ghost merged commit b70aa22 into master Feb 16, 2018
@ghost ghost deleted the getTextOfPropertyName branch February 16, 2018 18:49
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant