Skip to content

[Transforms] Extract transformFiles call from printFile #8929

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 8 commits into from
Jun 3, 2016

Conversation

rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Jun 2, 2016

To reduce the number of closures created, this moves the call to transformFiles out of printFile and into the main body of printFiles.

@rbuckton rbuckton added the Domain: Transforms Relates to the public transform API label Jun 2, 2016
export function getSourceTreeNodeOfType<T extends Node>(node: T, nodeTest: (node: Node) => node is T): T {
const source = getSourceTreeNode(node);
return source && nodeTest(source) ? source : undefined;
export function getOriginalSourceFiles(sourceFiles: SourceFile[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this function used? it doesn't seems to be use in this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return !isDeclarationFile(sourceFile) && !isExternalModule(sourceFile);
}

export function forEachEmitFile(host: EmitHost, sourceFiles: SourceFile[],
Copy link
Contributor

Choose a reason for hiding this comment

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

It will be great if you could comment the difference between forEachEmitFile forEachExpectedEmitFile. they are quite similar at a glace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add comments.

@yuit
Copy link
Contributor

yuit commented Jun 3, 2016

This is not related to this change. but just wonder why we still keep getNodeEmitFlags in the TransformationContext since it is just return node.emitFlags? we don't we change every thing to use node.emitFlags. It is just a bit confusing since in the "printers.ts" we use node.emitFlags while in some places (such as destructuring.ts we still use getNodeEmitFlags from the context

@yuit
Copy link
Contributor

yuit commented Jun 3, 2016

Clarification: printer.ts on line 224. the comment above (line 223) should be just
"// Print the souce files" instead of "// Transform and print the source files" because we have done all the transformation in line 150

@yuit
Copy link
Contributor

yuit commented Jun 3, 2016

Also to clarify: this change remove printer from being part of the chain of transformer and become it separate part of the pipeline that take in transformed source files and print them?

@rbuckton
Copy link
Contributor Author

rbuckton commented Jun 3, 2016

we don't we change every thing to use node.emitFlags

I have some stashed progress on this. The tricky part is ensuring we always read via the property, but write via setNodeEmitFlags. I've considered marking emitFlags as readonly on Node.

Clarification: printer.ts on line 224...

I'll update, thanks.

...this change remove printer from being part of the chain...

That is correct.

@yuit
Copy link
Contributor

yuit commented Jun 3, 2016

Gotcha. Others than the comments, the change looks good. 👍

@rbuckton rbuckton merged commit 877e3c4 into transforms Jun 3, 2016
@rbuckton rbuckton deleted the transforms-extractFromPrinter branch June 3, 2016 18:52
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Transforms Relates to the public transform API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants