Skip to content

Allow resolving of ES5 default values in functions#25048

Closed
alxhub wants to merge 1 commit into
angular:masterfrom
alxhub:ngtsc-resolve-es5
Closed

Allow resolving of ES5 default values in functions#25048
alxhub wants to merge 1 commit into
angular:masterfrom
alxhub:ngtsc-resolve-es5

Conversation

@alxhub
Copy link
Copy Markdown
Member

@alxhub alxhub commented Jul 23, 2018

No description provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this be reworded to This list may have been filtered (...) to indicate the ReflectionHost is expected to do that. Currently it reads to me as if consumers of ReflectionHost are responsible for doing so, in which it doesn't make much sense to tell here wat consumers can do with an array :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll fix this in my PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps stick some triple back ticks around this.

ngtsc's static resolver can evaluate function calls where parameters
have default values. In TypeScript code these default values live on the
function definition, but in ES5 code the default values are represented
by statements in the function body.

A new ReflectionHost method getDefinitionOfFunction() abstracts over
this difference, and allows the static reflector to more accurately
evaluate ES5 code.
@alxhub alxhub force-pushed the ngtsc-resolve-es5 branch from 789c2ef to b185031 Compare July 23, 2018 21:47
@mary-poppins
Copy link
Copy Markdown

You can preview b185031 at https://pr25048-b185031.ngbuilds.io/.

Copy link
Copy Markdown
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM (linting issues aside)

* A current limitation is that this metadata has no representation for shorthand assignment of
* parameter objects in the function signature.
*
* @param node a TypeScript `ts.Declaration` node representing the function over which to reflect.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The param's name is fn 😁

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll fix this in my PR

return {
node,
body: node.body !== undefined ? Array.from(node.body.statements) : null,
parameters: node.parameters.map(node => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider renaming the second node to paramNode to avoid confusion below.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll fix this in my PR

@alxhub
Copy link
Copy Markdown
Member Author

alxhub commented Jul 27, 2018

@petebacondarwin will merge this along with the ngcc implementation.

@alxhub alxhub closed this Jul 27, 2018
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants