Skip to content

feat(compiler-cli): automatically lower lambda expressions in metadata - #18390

Merged
alxhub merged 2 commits into
angular:masterfrom
chuckjaz:expression-lowering-3
Jul 31, 2017
Merged

feat(compiler-cli): automatically lower lambda expressions in metadata#18390
alxhub merged 2 commits into
angular:masterfrom
chuckjaz:expression-lowering-3

Conversation

@chuckjaz

@chuckjaz chuckjaz commented Jul 27, 2017

Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

When a lambda expression is used in an Angular decorator an error is reported during AOT requesting the user rewrite the expression.

What is the new behavior?

The expression is automatically rewritten to an exported reference.

NOTE: This is not yet enabled in ngc as it requires the transform based compiler which is not yet the default.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@chuckjaz
feat(tsc-wrapped): allow values to be substituted by collector clients
5e6e5af 
Also reenabled tests that were unintentionally disabled when they were
moved from tools/@angular.
@chuckjaz
chuckjaz force-pushed the expression-lowering-3 branch from 62a7fe9 to 276e5c6 Compare July 27, 2017 22:12
vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/src/transformers/lower_expressions.ts Outdated

@vicb vicb Jul 27, 2017

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.

consider using Math.min and ensure values are num

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/src/transformers/lower_expressions.ts Outdated

@vicb vicb Jul 27, 2017

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.

trailing comma

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/src/transformers/lower_expressions.ts Outdated

@vicb vicb Jul 27, 2017

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.

handle function expression

@mary-poppins

mary-poppins commented Jul 27, 2017

Copy link
Copy Markdown

You can preview 62a7fe9 at https://pr18390-62a7fe9.ngbuilds.io/.

@mary-poppins

mary-poppins commented Jul 27, 2017

Copy link
Copy Markdown

You can preview 276e5c6 at https://pr18390-276e5c6.ngbuilds.io/.

@chuckjaz
chuckjaz force-pushed the expression-lowering-3 branch from 276e5c6 to fcdfda3 Compare July 27, 2017 22:52
@mary-poppins

mary-poppins commented Jul 27, 2017

Copy link
Copy Markdown

You can preview fcdfda3 at https://pr18390-fcdfda3.ngbuilds.io/.

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/src/transformers/lower_expressions.ts Outdated

@vicb vicb Jul 27, 2017

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.

typo: references

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/src/transformers/lower_expressions.ts Outdated

@vicb vicb Jul 27, 2017

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.

Should with move that to the transform itself ?
(seems like this leaks an impl detail to me)

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/test/ngc_spec.ts Outdated

@vicb vicb Jul 27, 2017

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.

typo: should able to multiple expressions

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/test/ngc_spec.ts Outdated

@vicb vicb Jul 27, 2017

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.

nit: , 'utf8') is the default IIRC

@chuckjaz chuckjaz Jul 28, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually it returns a Buffer if you don't specify a character set.

@vicb vicb Jul 28, 2017

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.

oh right

vicb
vicb reviewed Jul 27, 2017
View reviewed changes
Comment thread packages/compiler-cli/src/transformers/lower_expressions.ts Outdated

@vicb vicb Jul 27, 2017

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.

createConst ?

@chuckjaz chuckjaz Jul 28, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Change this to be const by adding a const modifier to the declaration list.

@chuckjaz
chuckjaz force-pushed the expression-lowering-3 branch from fcdfda3 to 30bd96d Compare July 28, 2017 21:15
chuckjaz
chuckjaz commented Jul 28, 2017
View reviewed changes
Comment thread packages/compiler-cli/test/transformers/lower_expressions_spec.ts
function normalizeResult(result: string): string {
// Remove TypeScript prefixes
// Remove new lines
// Squish adjacent spaces

@chuckjaz chuckjaz Jul 28, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Squish and squash are pretty much synonyms.

@chuckjaz chuckjaz added the action: merge The PR is ready for merge by the caretaker label Jul 28, 2017
@mary-poppins

mary-poppins commented Jul 28, 2017

Copy link
Copy Markdown

You can preview 30bd96d at https://pr18390-30bd96d.ngbuilds.io/.

@alxhub
alxhub merged commit b6c4af6 into angular:master Jul 31, 2017
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 12, 2019

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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@vicb vicb vicb approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@chuckjaz @mary-poppins @vicb @alxhub @googlebot