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(core): add input and output filtering for host directives #47536

Closed

Conversation

crisbeto
Copy link
Member

Adds the logic that will filter out unexposed inputs/outputs and apply the aliases that the author specified when writing the host directives.

@crisbeto crisbeto added area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Sep 24, 2022
@crisbeto crisbeto added this to the v15 feature freeze blockers milestone Sep 24, 2022
tView: TView, lView: LView,
tNode: TElementNode|TContainerNode|TElementContainerNode): DirectiveDef<unknown>[]|null {
tView: TView, lView: LView, tNode: TElementNode|TContainerNode|TElementContainerNode):
[matches: DirectiveDef<unknown>[], definitionMap: HostDirectiveDefinitionMap|null]|null {
Copy link
Member Author

@crisbeto crisbeto Sep 24, 2022

Choose a reason for hiding this comment

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

I'm not a fan of having this return an array when it might not need to, but I couldn't find a better way of producing two results from findDirectiveDefMatches. Some of the considered alternatives:

  • Having a separate function that is called inside resolveDirectives that produces the map. The problem with this is that we don't have a way of distinguishing which matches came from host directives versus selector matching.
  • Create the definition map inside resolveDirectives and pass it into findDirectiveDefMatches. This would work, but we might produce the binding map and not use it.
  • Inverting the selector matching logic. We decided against doing so here refactor(core): generate initial inputs from inputs map #47228 (review).
  • Have findDirectiveDefMatches return something like DirectiveDef[] | {matches: DirectiveDef[], mappings: HostDirectiveDefinitionMap} | null. This should be viable, but I'm not sure about the perf implications of generating object literals instead of arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as an idea, we can pass an object as an accumulator into the findDirectiveDefMatches function, for example:

const accumulator = {matchedDefs: null, hostDirectiveDefs: null};
collectDirectiveDefMatches(accumulator, ...);

I think the current approach is fine too, we do have the assertFirstCreatePass at the very beginning of the function, so we guarantee to invoke the function only once per component (at creation time), so it should not affect the performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was one of the approaches I was considering, but I decided against it because we'd be creating the object even if we don't need it. That being said, I feel like I've been microoptimizing a bit here.

@crisbeto crisbeto force-pushed the host-directives-inputs-outputs branch from f962853 to 31f98fc Compare September 24, 2022 08:06
* Mapping between a directive that was used as a host directive
* and the configuration that was used to define it as such.
*/
export type HostDirectiveDefinitionMap = Map<DirectiveDef<unknown>, HostDirectiveDef>;
Copy link
Member Author

Choose a reason for hiding this comment

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

I was struggling a bit with how to name this so I'm open to suggestions.

@crisbeto crisbeto added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 24, 2022
@crisbeto crisbeto marked this pull request as ready for review September 24, 2022 08:23
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved
@crisbeto
Copy link
Member Author

The feedback has been addressed.

@crisbeto crisbeto force-pushed the host-directives-inputs-outputs branch from 31f98fc to e35a2b2 Compare September 27, 2022 07:05
@@ -412,12 +417,27 @@ export interface HostDirectiveDef<T = unknown> {
directive: Type<T>;

/** Directive inputs that have been exposed. */
inputs: {[publicName: string]: string};
inputs: HostDirectiveBindingMap;

Choose a reason for hiding this comment

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

If I'm not mistaken, the inputs / outputs field is applicable to directives that don't have any host directives applied to them, right? If so, I find the name HostDirectiveBindingMap misleading here. It is more about input / output aliasing, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does two things:

  1. It acts as an allowlist for the inputs/outputs that are exposed.
  2. It's used to remap the public name.

I was struggling with how to capture this in the name of the symbol so I went with something more generic like "binding".

@crisbeto
Copy link
Member Author

I ended up squashing the commits so it's easier to rebase the changes from #47530.

Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM overall.

Personally I had a different design on my mind where the feature is patching setInput and more of the code goes into the feature itself. But this would mean substantial re-design / re-write.

@pullapprove pullapprove bot requested a review from alxhub September 28, 2022 08:02
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: size-tracking
Reviewed-for: fw-core

@crisbeto crisbeto removed the request for review from alxhub September 28, 2022 08:35
@crisbeto crisbeto 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 Sep 28, 2022
@crisbeto crisbeto removed the request for review from AndrewKushnir September 28, 2022 08:35
Adds the logic that will filter out unexposed inputs/outputs and apply the aliases that the author specified when writing the host directives.
@crisbeto crisbeto force-pushed the host-directives-inputs-outputs branch from 00f83ad to a3571ce Compare September 29, 2022 20:47
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 76a8c68.

pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
…ngular#47536)

Adds the logic that will filter out unexposed inputs/outputs and apply the aliases that the author specified when writing the host directives.

PR Close angular#47536
pterratpro pushed a commit to pterratpro/angular-fr that referenced this pull request Oct 4, 2022
…ngular#47536)

Adds the logic that will filter out unexposed inputs/outputs and apply the aliases that the author specified when writing the host directives.

PR Close angular#47536
@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 Oct 30, 2022
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: core Issues related to the framework runtime target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants