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

fix(ivy): wrap functions from "providers" in parentheses in Closure mode #33609

Conversation

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Nov 6, 2019

Due to the fact that Tsickle runs between analyze and transform phases in Angular, Tsickle may transform nodes (add comments with type annotations for Closure) that we captured during the analyze phase. As a result, some patterns where a function is returned from another function may trigger automatic semicolon insertion, which breaks the code (makes functions return undefined instead of a function). In order to avoid the problem, this commit updates the code to wrap all functions in some expression ("privders" and "viewProviders") in parentheses. More info can be found in Tsickle source code here: https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
@ngbot ngbot bot modified the milestone: needsTriage Nov 6, 2019
@googlebot googlebot added the cla: yes label Nov 6, 2019
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1662_providers_with_closure branch from 623ba11 to face365 Nov 6, 2019
@AndrewKushnir AndrewKushnir requested a review from alxhub Nov 6, 2019
@AndrewKushnir AndrewKushnir marked this pull request as ready for review Nov 6, 2019
@AndrewKushnir AndrewKushnir requested review from angular/fw-compiler as code owners Nov 6, 2019
Copy link
Member

petebacondarwin left a comment

LTGM

  • typo in the commit message: privders => providers
  • a handful of nits - nothing blocking
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts Outdated Show resolved Hide resolved
packages/compiler-cli/test/ngtsc/ngtsc_spec.ts Outdated Show resolved Hide resolved
}
return visited;
};
return (node: ts.Expression) => ts.visitEachChild(node, visitor, context);

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 6, 2019

Member
Suggested change
return (node: ts.Expression) => ts.visitEachChild(node, visitor, context);
return node => ts.visitEachChild(node, visitor, context);
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1662_providers_with_closure branch from e7a7f9e to 4f6b3ca Nov 14, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Nov 14, 2019

@petebacondarwin @alxhub thanks for the review! I've updated this PR based on the comments:

  • kept more explicit types and parameters as Alex proposed
  • updates description and restructured tests as Pete suggested

Could you please have a quick look again and let me know if you have additional feedback?

Thank you.

Due to the fact that Tsickle runs between analyze and transform phases in Angular, Tsickle may transform nodes (add comments with type annotations for Closure) that we captured during the analyze phase. As a result, some patterns where a function is returned from another function may trigger automatic semicolon insertion, which breaks the code (makes functions return `undefined` instead of a function). In order to avoid the problem, this commit updates the code to wrap all functions in some expression ("privders" and "viewProviders") in parentheses. More info can be found in Tsickle source code here: https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021
@AndrewKushnir AndrewKushnir force-pushed the AndrewKushnir:FW-1662_providers_with_closure branch from 4f6b3ca to 4de646c Nov 19, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor Author

AndrewKushnir commented Nov 19, 2019

@alxhub
alxhub approved these changes Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 20, 2019
…ode (#33609)

Due to the fact that Tsickle runs between analyze and transform phases in Angular, Tsickle may transform nodes (add comments with type annotations for Closure) that we captured during the analyze phase. As a result, some patterns where a function is returned from another function may trigger automatic semicolon insertion, which breaks the code (makes functions return `undefined` instead of a function). In order to avoid the problem, this commit updates the code to wrap all functions in some expression ("privders" and "viewProviders") in parentheses. More info can be found in Tsickle source code here: https://github.com/angular/tsickle/blob/d7974262571c8a17d684e5ba07680e1b1993afdd/src/jsdoc_transformer.ts#L1021

PR Close #33609
@alxhub alxhub closed this in fc2f6b8 Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.