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.

Sign up for GitHub

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

Jump to bottom

fix(ivy): match directives on bindings and element outputs #25614

Closed
wants to merge 1 commit into from
Closed

fix(ivy): match directives on bindings and element outputs #25614

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

Closes #23560

This PR makes sure that directives are matching on:

  • bindings ex.: <div [myDirective]="exp"> / <ng-template [myDirective]="exp">
  • outputs on elements, ex.: <div (myDirective)="doSth()">

To do the above we need to do proper directives matching on the compiler side (see ) and generate attributes for matching purposes in the runtime (see).

Matching on <ng-template> outputs (ex.: <ng-template (myDirective)="doSth()">) will be done in a follow-up PR as it requires(?) re-working of the listener(...) instruction.

@pkozlowski-opensource pkozlowski-opensource force-pushed the directives_matching_on_bindings_outputs branch from 904b664 to ecaf96e Compare August 22, 2018 13:09
@pkozlowski-opensource pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Aug 22, 2018
@mary-poppins
Copy link

You can preview ecaf96e at https://pr25614-ecaf96e.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource force-pushed the directives_matching_on_bindings_outputs branch from ecaf96e to e793079 Compare August 22, 2018 14:20
@mary-poppins
Copy link

You can preview e793079 at https://pr25614-e793079.ngbuilds.io/.

@pkozlowski-opensource
Copy link
Member Author

Regarding the CircleCI job failing - I can reproduce the same issue on master.

Did a quick error analysis and it fails since component def seem not to be generated / component not patched. Happens in the AOT mode only, JIT is fine. Not sure how to troubleshoot those, though...

alfaproject
alfaproject reviewed Aug 22, 2018
View reviewed changes
packages/compiler/src/render3/view/template.ts
const attrsExprs: o.Expression[] = [];
template.attributes.forEach(
(a: t.TextAttribute) => { attrsExprs.push(asLiteral(a.name), asLiteral(a.value)); });
attrsExprs.push(...this.prepareSelectOnlyAttrs(template.inputs, template.outputs));
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI spreading generates quite a bit of boilerplate in ES5 target

Copy link
Member

Choose a reason for hiding this comment

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

What kind of boilerplate? Isn't it just a.push.apply(a, b)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus it should be mostly run on Node so ES2015+ target...

JoostK
JoostK reviewed Aug 22, 2018
View reviewed changes
packages/core/src/render3/jit/directive.ts Outdated
@@ -82,7 +82,8 @@ export function compileComponent(type: Type<any>, metadata: Component): void {

// If component compilation is async, then the @NgModule annotation which declares the
// component may execute and set an ngSelectorScope property on the component type. This
// allows the component to patch itself with directiveDefs from the module after it finishes
// allows the component to patch itself with directiveDefs from the module after it
// finishes
// compiling.
Copy link
Member

Choose a reason for hiding this comment

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

compiling could now be moved to above line.

packages/core/src/render3/jit/environment.ts Outdated
@@ -49,6 +49,7 @@ export const angularCoreEnv: {[name: string]: Function} = {
'ɵnamespaceHTML': r3.namespaceHTML,
'ɵnamespaceMathML': r3.namespaceMathML,
'ɵnamespaceSVG': r3.namespaceSVG,
'ɵamSelectOnly': r3.AttributeMarker.SelectOnly,
Copy link
Member

Choose a reason for hiding this comment

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

We'd also need to export ɵamSelectOnly for AOT compilations. FYI, styling constants are inlined by the compiler currently.

angular/packages/compiler/src/render3/view/template.ts

Line 426 in e793079

initialStyleDeclarations.push(o.literal(core.InitialStylingFlags.VALUES_MODE));

angular/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts

Line 89 in e793079

const $e0_styling$ = ["opacity","width","height",${InitialStylingFlags.VALUES_MODE},"opacity","1"];

I think it would be better to inline it everywhere, or nowhere at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh, I think in-lining it is a better option. Will change this, thnx!

@pkozlowski-opensource pkozlowski-opensource force-pushed the directives_matching_on_bindings_outputs branch from e793079 to 5e58983 Compare August 23, 2018 08:44
@mary-poppins
Copy link

You can preview 5e58983 at https://pr25614-5e58983.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource force-pushed the directives_matching_on_bindings_outputs branch from 5e58983 to e748536 Compare August 27, 2018 08:43
@mary-poppins
Copy link

You can preview e748536 at https://pr25614-e748536.ngbuilds.io/.

@pkozlowski-opensource pkozlowski-opensource force-pushed the directives_matching_on_bindings_outputs branch from e748536 to 314dd13 Compare August 27, 2018 09:33
@mary-poppins
Copy link

You can preview 314dd13 at https://pr25614-314dd13.ngbuilds.io/.

@pkozlowski-opensource
Copy link
Member Author

ci/circleci: integration_test is failing since master is broken - it should be fixed with #25661

@mhevery mhevery force-pushed the directives_matching_on_bindings_outputs branch from 314dd13 to c154631 Compare August 27, 2018 16:59
@mhevery
Copy link
Contributor

mhevery commented Aug 27, 2018

http://test/OCL:210383039:BASE:210383131:1535389107215:fc3eb56d

@mary-poppins
Copy link

You can preview c154631 at https://pr25614-c154631.ngbuilds.io/.

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 27, 2018
@matsko matsko closed this in 6a0f78f Aug 27, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@pkozlowski-opensource @FrederikSchlemmer
fix(ivy): match directives on bindings and element outputs (angular#2…
b9f16fd 
…5614)

Closes angular#23560

PR Close angular#25614
@angular-automatic-lock-bot
Copy link

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.
Reviewers

@alfaproject alfaproject alfaproject left review comments

@JoostK JoostK JoostK left review comments

@gkalpak gkalpak gkalpak left review comments

@mhevery mhevery mhevery approved these changes

Assignees

@mhevery mhevery

@alxhub alxhub

Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[IVY] Event bindings should trigger directives
8 participants
@pkozlowski-opensource @mary-poppins @mhevery @alfaproject @JoostK @gkalpak @alxhub @googlebot