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

refactor(compiler-cli): specify whether a used directive is a component in partial component declaration #41104

Closed

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Mar 6, 2021

The partial declaration of a component includes the list of directives
that are used in its template, including some metadata of the directive
which can be used during actual compilation of the component. This
commit introduces an additional metadata flag isComponent such that
template compilation can use this information in the future.

@JoostK JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler compiler: linker labels Mar 6, 2021
@JoostK JoostK added this to In progress in ng-linker via automation Mar 6, 2021
@ngbot ngbot bot modified the milestone: Backlog Mar 6, 2021
@google-cla google-cla bot added the cla: yes label Mar 6, 2021
@JoostK JoostK moved this from In progress to Review in progress in ng-linker Mar 6, 2021
Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Here's a crazy idea... Instead of marking each component with isComponent, how about we change the declaration to have two properties directives and components, which get consolidated back together in the linker?

@JoostK JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 8, 2021
@JoostK JoostK moved this from Review in progress to In progress in ng-linker Mar 8, 2021
… in partial declaration

The partial declaration of a component includes the list of directives
that are used in its template, including some metadata of the directive
which can be used during actual compilation of the component. Used
components are currently part of this list, as components are also
directives. This commit splits the used components into a dedicate
property in the partial declaration, which allows for template
compilation to optimize the generated code for components.
@JoostK JoostK force-pushed the linker/used-directives-is-component branch from ff307dc to 82f0bd6 Compare March 13, 2021 20:15
@JoostK JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 13, 2021
@JoostK JoostK moved this from In progress to Review in progress in ng-linker Mar 13, 2021
@JoostK JoostK added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Mar 16, 2021
Comment on lines +77 to +105
const collectUsedDirectives =
(directives: AstValue<R3DeclareUsedDirectiveMetadata, TExpression>[]) => {
return directives.map(directive => {
const directiveExpr = directive.getObject();
const type = directiveExpr.getValue('type');
const selector = directiveExpr.getString('selector');

let typeExpr = type.getOpaque();
const forwardRefType = extractForwardRef(type);
if (forwardRefType !== null) {
typeExpr = forwardRefType;
declarationListEmitMode = DeclarationListEmitMode.Closure;
}

return {
type: typeExpr,
selector: selector,
inputs: directiveExpr.has('inputs') ?
directiveExpr.getArray('inputs').map(input => input.getString()) :
[],
outputs: directiveExpr.has('outputs') ?
directiveExpr.getArray('outputs').map(input => input.getString()) :
[],
exportAs: directiveExpr.has('exportAs') ?
directiveExpr.getArray('exportAs').map(exportAs => exportAs.getString()) :
null,
};
});
};
Copy link
Member

Choose a reason for hiding this comment

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

Ugh! I'd love to pull this out into a separate utility function at the bottom of this file.
I tried to do it myself when I originally wrote this block of code.
But the update to declarationListEmitMode makes it awkward to be in a separate function.
😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, declarationListEmitMode made this into a local closure.

@JoostK JoostK added action: rerun CI at HEAD and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 16, 2021
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit labels Mar 16, 2021
@AndrewKushnir
Copy link
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Mar 17, 2021
ng-linker automation moved this from Review in progress to Done Mar 17, 2021
JoostK added a commit to JoostK/angular that referenced this pull request Mar 26, 2021
…ial component declaration

In angular#41104 the list of used directives was split into two arrays of used
directives and components, but the JIT side was not updated. This commit
fixes the JIT integration by including the list of used components.

Fixes angular#41318
JoostK added a commit to JoostK/angular that referenced this pull request Mar 30, 2021
…ial component declaration

In angular#41104 the list of used directives was split into two arrays of used
directives and components, but the JIT side was not updated. This commit
fixes the JIT integration by including the list of used components.

Fixes angular#41318
JoostK added a commit to JoostK/angular that referenced this pull request Mar 31, 2021
…ial component declaration

In angular#41104 the list of used directives was split into two arrays of used
directives and components, but the JIT side was not updated. This commit
fixes the JIT integration by including the list of used components.

Fixes angular#41318
alxhub pushed a commit that referenced this pull request Apr 1, 2021
…ial component declaration (#41353)

In #41104 the list of used directives was split into two arrays of used
directives and components, but the JIT side was not updated. This commit
fixes the JIT integration by including the list of used components.

Fixes #41318

PR Close #41353
TeriGlover pushed a commit to TeriGlover/angular that referenced this pull request Apr 5, 2021
…ial component declaration (angular#41353)

In angular#41104 the list of used directives was split into two arrays of used
directives and components, but the JIT side was not updated. This commit
fixes the JIT integration by including the list of used components.

Fixes angular#41318

PR Close angular#41353
@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 Apr 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes compiler: linker target: major This PR is targeted for the next major release
Projects
No open projects
ng-linker
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants