Skip to content

fix(core): set correct context for inject() for component ctors#45991

Closed
alxhub wants to merge 1 commit intoangular:mainfrom
alxhub:fix-inject
Closed

fix(core): set correct context for inject() for component ctors#45991
alxhub wants to merge 1 commit intoangular:mainfrom
alxhub:fix-inject

Conversation

@alxhub
Copy link
Member

@alxhub alxhub commented May 13, 2022

The inject() function was introduced with Ivy to support imperative
injection in factory/constructor contexts, such as directive or service
constructors as well as factory functions defined in @Injectable or
InjectionToken. However, inject() in a component/directive constructor
did not work due to a flaw in the logic for creating the internal factory
for components/directives.

The original intention of this logic was to keep ɵɵdirectiveInject tree-
shakable for applications which don't use any component-level DI. However,
this breaks the inject() functionality for component/directive
constructors.

This commit fixes that issue and adds tests for all the various cases in
which inject() should function. As a result ɵɵdirectiveInject is no
longer tree-shakable, but that's totally acceptable as any application that
uses *ngIf or *ngFor already contains this function. It's possible to
change how inject() works to restore this tree-shakability if needed.

@alxhub alxhub marked this pull request as ready for review May 13, 2022 17:42
@alxhub alxhub added target: patch This PR is targeted for the next patch release area: core Issues related to the framework runtime core: di labels May 13, 2022
@ngbot ngbot bot added this to the Backlog milestone May 13, 2022
The `inject()` function was introduced with Ivy to support imperative
injection in factory/constructor contexts, such as directive or service
constructors as well as factory functions defined in `@Injectable` or
`InjectionToken`. However, `inject()` in a component/directive constructor
did not work due to a flaw in the logic for creating the internal factory
for components/directives.

The original intention of this logic was to keep `ɵɵdirectiveInject` tree-
shakable for applications which don't use any component-level DI. However,
this breaks the `inject()` functionality for component/directive
constructors.

This commit fixes that issue and adds tests for all the various cases in
which `inject()` should function. As a result `ɵɵdirectiveInject` is no
longer tree-shakable, but that's totally acceptable as any application that
uses `*ngIf` or `*ngFor` already contains this function. It's possible to
change how `inject()` works to restore this tree-shakability if needed.
Copy link
Contributor

@jessicajaniuk jessicajaniuk 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, fw-core

@jessicajaniuk jessicajaniuk removed the request for review from pkozlowski-opensource May 13, 2022 18:03
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels May 13, 2022
@ngbot
Copy link

ngbot bot commented May 13, 2022

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/angular: size" is failing
    pending status "ci/circleci: test_aio" is pending
    pending status "ci/circleci: test_aio_local" is pending
    pending status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@alxhub
Copy link
Member Author

alxhub commented May 13, 2022

Caretaker: CI size difference is as expected.

@jessicajaniuk jessicajaniuk added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels May 13, 2022
@alxhub alxhub added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels May 13, 2022
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 9c1735b.

jessicajaniuk pushed a commit that referenced this pull request May 13, 2022
The `inject()` function was introduced with Ivy to support imperative
injection in factory/constructor contexts, such as directive or service
constructors as well as factory functions defined in `@Injectable` or
`InjectionToken`. However, `inject()` in a component/directive constructor
did not work due to a flaw in the logic for creating the internal factory
for components/directives.

The original intention of this logic was to keep `ɵɵdirectiveInject` tree-
shakable for applications which don't use any component-level DI. However,
this breaks the `inject()` functionality for component/directive
constructors.

This commit fixes that issue and adds tests for all the various cases in
which `inject()` should function. As a result `ɵɵdirectiveInject` is no
longer tree-shakable, but that's totally acceptable as any application that
uses `*ngIf` or `*ngFor` already contains this function. It's possible to
change how `inject()` works to restore this tree-shakability if needed.

PR Close #45991
template: '{{value}}',
})
class TestCmp {
cdr = inject(ChangeDetectorRef);
Copy link
Contributor

@splincode splincode May 18, 2022

Choose a reason for hiding this comment

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

@alxhub Hello, thank you for this PR! Really great job!

I want to say you about, is this working with custom decorator?

const Autowired = (target: any, memberName: string) => {
  Object.defineProperty(target, memberName, {
    set: (newValue: any) => {
      target[memberName] = newValue;
    },
    get: () => inject(ChangeDetectorRef)
  });
};

@Component({
    standalone: true,
    selector: 'test-cmp',
    template: '{{value}}',
  })
  class TestCmp {
    @Autowired() cdr!: ChangeDetectorRef;
  }

Copy link
Member

Choose a reason for hiding this comment

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

No, that particular code wouldn't work as inject is not called during construction, but only in the property's getter. The injection context won't be available at that point in time.

Copy link
Contributor

Choose a reason for hiding this comment

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

What code is then valid for writing your own decorator? Tell me please

Copy link
Member

Choose a reason for hiding this comment

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

Property decorators don't get to add logic to run during construction, so you can't do that. There's probably ways to get things to work with a supporting class decorator, but I wouldn't recommend that.

Really just use

readonly cdr = inject(ChangeDetectorRef);

which gets you type-safe code without size nor runtime overhead and further magic (you have an additional problem to solve w.r.t. the token to inject, which in your example is hardcoded as ChangeDetectorRef in Autowired).

@jessicajaniuk jessicajaniuk reopened this May 18, 2022
@jessicajaniuk
Copy link
Contributor

Looks like this caused a breakage. Reverting and re-opening.

jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:
* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:
* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
… using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 18, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 19, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 19, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 20, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 20, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
jelbourn added a commit to jelbourn/components that referenced this pull request May 20, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
mmalerba pushed a commit to angular/components that referenced this pull request May 23, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
mmalerba pushed a commit to angular/components that referenced this pull request May 23, 2022
This change switches the cdk menu from using constructor injection to using the `inject` function. This was enabled by angular/angular#45991. This approach has a number of advantages:

* We can add and remove injectables without changing the public constructor signature
* It reduces the boilerplate of passing arguments through `super`
* It avoids long/messy constructor blocks in favor of simpler member definitions.
* This sidesteps a common pitfall of making `@Optional` injection (`null` if not provided) as an optional parameter (`undefined` if not provided)`.
@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 Jun 18, 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 core: di merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants