Skip to content

[Master] fix 16407 - LS in binding element of object binding pattern #16534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Aug 11, 2017

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Jun 14, 2017

Fix #16407
Because identifier in bindingElement in objectBindingPattern is considered declaration name, previously it will just return its symbol. So when perform goto-definition, it will simply just stay where it is

function bar<T>(onfulfilled: (value: T) => void) {
  return undefined;
}

interface Test {
  prop2: number
}
bar<Test>(({pr/*gotoDef*/op2})=>{});  // just stay at the same place

However, it is desirable to actually goto the prop2 in an interface. See: master...master-fix16407#diff-3c46922136bdd65e7a2c2d12db87bbe5....

  • Updates change in .symbols files and .types file as we will refer the symbol to the object type now

@weswigham
Copy link
Member

weswigham commented Aug 8, 2017

I've iterated on yui's change - it no longer changes how we match up symbols in the checker, as that was breaking flow control analysis. Now, it does for object binding patterns a similar action as it does for property names in an object literal. The baseline changes are now nonexistent, and it is just new services functionality.

@weswigham weswigham self-assigned this Aug 8, 2017
@@ -76,6 +76,26 @@ namespace ts.GoToDefinition {
declaration => createDefinitionInfo(declaration, shorthandSymbolKind, shorthandSymbolName, shorthandContainerName));
}

// If the node is an identifier in bindingelement of ObjectBindingPattern (Note: ArrayBindingPattern can only declaration)
Copy link
Member

@amcasey amcasey Aug 10, 2017

Choose a reason for hiding this comment

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

Would you mind rephrasing this a little? I find it a bit hard to follow.

// pr/*destination*/op1: number
// }
// bar<Test>(({pr/*goto*/op1})=>{});
if (isPropertyName(node) && isBindingElement(node.parent) && isObjectBindingPattern(node.parent.parent) && (node.parent.propertyName ? node.parent.propertyName === node : node.parent.name === node)) {
Copy link
Member

@amcasey amcasey Aug 10, 2017

Choose a reason for hiding this comment

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

Alternatively, node === (node.parent.propertyName || node.parent.name).

@weswigham
Copy link
Member

@amcasey done

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

LGTM

@weswigham weswigham merged commit d352e3b into master Aug 11, 2017
@weswigham weswigham deleted the master-fix16407 branch August 11, 2017 18:15
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
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.

Language Service for imported property through dynamic import
4 participants