Skip to content

feat(core): support injection token as predicate in queries #37506

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

Conversation

devversion
Copy link
Member

@devversion devversion commented Jun 9, 2020

Currently Angular internally already handles InjectionToken as
predicates for queries. This commit exposes this as public API as
developers already rely on this functionality but currently use
workarounds to satisfy the type constraints (e.g. as any).

We intend to make this public as it's low-effort to support, and
it's a significant key part for the use of light-weight tokens as
described in the upcoming guide: #36144.

In concrete, applications might use injection tokens over classes
for both optional DI and queries, because otherwise such references
cause classes to be always retained. This was also an issue in View
Engine, but now with Ivy, this pattern became worse, as factories are
directly attached to retained classes (ultimately ending up in the
production bundle, while being unused).

More details in the light-weight token guide and in: angular/angular-cli#16866.

Closes #21152. Related to #36144.

@devversion devversion changed the title feat(core): support injection tokens as predicate in queries feat(core): support injection token as predicate in queries Jun 9, 2020
@devversion devversion force-pushed the feat/core-support-injection-token-in-queries branch from 49fca35 to 8fb9a90 Compare June 9, 2020 13:16
@devversion devversion added area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Jun 9, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jun 9, 2020
@devversion devversion added core: queries hotlist: components team Related to Angular CDK or Angular Material action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 9, 2020
@devversion devversion marked this pull request as ready for review June 9, 2020 13:39
@pullapprove pullapprove bot requested review from alxhub, IgorMinar and jelbourn June 9, 2020 13:39
@devversion devversion force-pushed the feat/core-support-injection-token-in-queries branch from 8fb9a90 to 77c08b7 Compare June 9, 2020 13:57
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@pullapprove pullapprove bot requested a review from jelbourn June 9, 2020 18:53
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

Reviewed-for: public-api

@devversion devversion force-pushed the feat/core-support-injection-token-in-queries branch from 77c08b7 to 3f7685e Compare June 10, 2020 07:50
Currently Angular internally already handles `InjectionToken` as
predicates for queries. This commit exposes this as public API as
developers already relied on this functionality but currently use
workarounds to satisfy the type constraints (e.g. `as any`).

We intend to make this public as it's low-effort to support, and
it's a significant key part for the use of light-weight tokens as
described in the upcoming guide: angular#36144.

In concrete, applications might use injection tokens over classes
for both optional DI and queries, because otherwise such references
cause classes to be always retained. This was also an issue in View
Engine, but now with Ivy, this pattern became worse, as factories are
directly attached to retained classes (ultimately ending up in the
production bundle, while being unused).

More details in the light-weight token guide and in: https://github.com/angular/angular-cli/issues/16866.

Closes angular#21152. Related to angular#36144.
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM now! Thanks

(changing existing apis to use unknown could be a breaking change, so we can do so only in a major version and only with proper discussion/review. So I suggest we punt that discussion until master is open for v11)

Reviewed-for: global-approvers

presubmit

@IgorMinar IgorMinar removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 11, 2020
@IgorMinar IgorMinar added the action: merge The PR is ready for merge by the caretaker label Jun 11, 2020
@mhevery mhevery closed this in 97dc85b Jun 11, 2020
ngwattcos pushed a commit to ngwattcos/angular that referenced this pull request Jun 25, 2020
…37506)

Currently Angular internally already handles `InjectionToken` as
predicates for queries. This commit exposes this as public API as
developers already relied on this functionality but currently use
workarounds to satisfy the type constraints (e.g. `as any`).

We intend to make this public as it's low-effort to support, and
it's a significant key part for the use of light-weight tokens as
described in the upcoming guide: angular#36144.

In concrete, applications might use injection tokens over classes
for both optional DI and queries, because otherwise such references
cause classes to be always retained. This was also an issue in View
Engine, but now with Ivy, this pattern became worse, as factories are
directly attached to retained classes (ultimately ending up in the
production bundle, while being unused).

More details in the light-weight token guide and in: https://github.com/angular/angular-cli/issues/16866.

Closes angular#21152. Related to angular#36144.

PR Close angular#37506
@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 Jul 12, 2020
profanis pushed a commit to profanis/angular that referenced this pull request Sep 5, 2020
…37506)

Currently Angular internally already handles `InjectionToken` as
predicates for queries. This commit exposes this as public API as
developers already relied on this functionality but currently use
workarounds to satisfy the type constraints (e.g. `as any`).

We intend to make this public as it's low-effort to support, and
it's a significant key part for the use of light-weight tokens as
described in the upcoming guide: angular#36144.

In concrete, applications might use injection tokens over classes
for both optional DI and queries, because otherwise such references
cause classes to be always retained. This was also an issue in View
Engine, but now with Ivy, this pattern became worse, as factories are
directly attached to retained classes (ultimately ending up in the
production bundle, while being unused).

More details in the light-weight token guide and in: https://github.com/angular/angular-cli/issues/16866.

Closes angular#21152. Related to angular#36144.

PR Close angular#37506
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 cla: yes core: queries hotlist: components team Related to Angular CDK or Angular Material target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: InjectionToken is not assignable to ViewChildren or ContentChildren
4 participants