Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Sep 18, 2017

Just simplifying a method.

@ghost ghost requested a review from amcasey September 18, 2017 19:54
@@ -909,7 +909,7 @@ namespace ts.refactor.extractMethod {
}
}

function getStatementsOrClassElements(scope: Scope): ReadonlyArray<Statement> | ReadonlyArray<ClassElement> {
function getStatementsOrClassElements(scope: Scope): ReadonlyArray<Statement | ClassElement> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe these types are equivalent. Didn't the old version meant that the list was either all statements or all class elements?

Copy link
Author

@ghost ghost Sep 18, 2017

Choose a reason for hiding this comment

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

Yes, but we don't need that information and it breaks type inference on find for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I would leave a comment explaining why we're not using the more accurate type.

return child;
}
}
return find(getStatementsOrClassElements(scope), child =>
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a simple find loop, I think using a helper makes sense. In general though, in the absence of expression-level breakpoints (and I would be very happy to learn that these exist), loops are much easier to debug.

Copy link
Author

Choose a reason for hiding this comment

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

I think node.js recently started supporting column breakpoints -- should be in chrome too.

@ghost ghost merged commit 951974d into master Sep 19, 2017
@ghost ghost deleted the getNodeToInsertBefore branch September 19, 2017 15:27
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

2 participants