Skip to content
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

Introduce and consume suppressLeadingAndTrailingTrivia #19135

Merged
merged 2 commits into from
Oct 12, 2017

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Oct 12, 2017

When a range is extracted, the leading and trailing trivia stays at the extraction site.

Fixes #18626

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Looks OK to me, but I don't understand the relation between emitNode and node, so you should probably get another reviewer.

@@ -740,6 +740,8 @@ namespace ts.refactor.extractSymbol {
}

const { body, returnValueProperty } = transformFunctionBody(node, exposedVariableDeclarations, writes, substitutions, !!(range.facts & RangeFacts.HasReturn));
suppressLeadingAndTrailingTrivia(body);
Copy link
Member

Choose a reason for hiding this comment

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

So if you create a fresh node that re-use tokens, do you suppressLeadingAndTrailingTrivia on the new node or the old node?

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed offline, you probably want to suppress it on the new node. Note that the token shouldn't appear in two places in the tree - that will mess up the positions. You have to make a copy.

@sandersn
Copy link
Member

suppressLeadingAndTrailingTrivia fixes the duplicate comment problem I have in #18747. 👍

@amcasey
Copy link
Member Author

amcasey commented Oct 12, 2017

@weswigham gave me the impression he knew about emit flags. Perhaps he'd like to confirm that I'm not abusing the system too badly?

@weswigham
Copy link
Member

Nope, this is pretty much exactly what it is for and what we use it for inside transforms.

@amcasey
Copy link
Member Author

amcasey commented Oct 12, 2017

Everyone's happy?

@weswigham
Copy link
Member

Oh, also re @sandersn on the emitNode vs node distinction. Emit nodes are temporary - they get cleaned up after a transfom pass is done, so should only contain ephemeral information related to emitting the node.

@amcasey amcasey merged commit 98f04e6 into microsoft:master Oct 12, 2017
@amcasey amcasey deleted the GH18626 branch October 12, 2017 21:17
@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.

3 participants