fix(core): export inject() from @angular/core #22389

alxhub · 2018-02-22T23:39:44Z

inject() supports the ngInjectableDef-based configuration of the injector
(otherwise known as tree-shakeable services). It was missing from the
public API of @angular/core, this PR adds it.

The test added here is correct in theory, but may pass accidentally due
to the decorator side-effect replacing the inject() call at runtime. An
upcoming compiler PR will strip reified decorators from output entirely.

Fixes #22388.

CC @jelbourn

mary-poppins · 2018-02-22T23:52:29Z

You can preview a25dc34 at https://pr22389-a25dc34.ngbuilds.io/.

IgorMinar · 2018-02-23T00:06:47Z

tools/public_api_guard/core/core.d.ts

@@ -447,6 +447,9 @@ export interface HostDecorator {
 /** @stable */
 export declare const HostListener: HostListenerDecorator;

+/** @experimental */
+export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundValue?: undefined, flags?: InjectFlags): T;


shouldn't this be exported as ɵinject instead? I don't think that this needs to be a public api since only generated code needs access to it as far as I can tell.

Can you please clarify?

This is public API. InjectionToken depends on it being so. A user would write:

const TOKEN = new InjectionToken<Service>('tree-shakeable token!', { scope: Module, factory: () => new Service(inject(Dep)) });

inject() is how they interact with the injection system.

so we don't support providing dependencies via deps just like we do any any other provider?

Not with InjectionToken, because this isn't actually a provider definition. We can't support a provider here because that would require reflecting dependencies, which would bring in ReflectionCapabilities even in AOT mode.

IgorMinar · 2018-02-23T00:15:03Z

packages/core/src/di/injector.ts

+/**
+ * Injects a token from the currently active injector.
+ *
+ * This function must be used in the context of an `ngInjectableDef` factory function, and will


ngInjectableDef is not a public api so this comment won't make sense to a application developer reading this comment.

True, I'll reword it for now.

IgorMinar · 2018-02-23T00:16:23Z

tools/public_api_guard/core/core.d.ts

@@ -447,6 +447,9 @@ export interface HostDecorator {
 /** @stable */
 export declare const HostListener: HostListenerDecorator;

+/** @experimental */
+export declare function inject<T>(token: Type<T> | InjectionToken<T>, notFoundValue?: undefined, flags?: InjectFlags): T;


so we don't support providing dependencies via deps just like we do any any other provider?

mary-poppins · 2018-02-23T00:34:04Z

You can preview de32278 at https://pr22389-de32278.ngbuilds.io/.

IgorMinar · 2018-02-23T02:54:15Z

I spoke with Chuck and we don't think that reflection capabilities would be needed if we used deps. Can you please sync up with him and come to an agreement? My preference is to keep this api private (for now) unless there is a strong reason to make it public.

…

On Thu, Feb 22, 2018, 4:34 PM Mary Poppins ***@***.***> wrote: You can preview de32278 <de32278> at https://pr22389-de32278.ngbuilds.io/. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#22389 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANM6Ln2M2RttuDqam_rPXVaMtCTrkM3ks5tXgeCgaJpZM4SQKns> .

mary-poppins · 2018-02-23T17:14:43Z

You can preview 50a9b9c at https://pr22389-50a9b9c.ngbuilds.io/.

ngbot · 2018-02-23T17:22:22Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required status "continuous-integration/travis-ci/pr"
     missing required status "code-review/pullapprove"
     status "google3" is pending
     status "ci/circleci: build" is pending
     status "ci/circleci: lint" is pending
     1 pending code review

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 master, please try rebasing to master 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.

All comments addressed, needs re-review.

IgorMinar

thanks

mary-poppins · 2018-02-23T17:35:00Z

You can preview abc6001 at https://pr22389-abc6001.ngbuilds.io/.

mary-poppins · 2018-02-26T16:56:36Z

You can preview 6aac2aa at https://pr22389-6aac2aa.ngbuilds.io/.

mary-poppins · 2018-02-26T17:03:25Z

You can preview 4d3931e at https://pr22389-4d3931e.ngbuilds.io/.

mary-poppins · 2018-02-26T19:23:52Z

You can preview ef54180 at https://pr22389-ef54180.ngbuilds.io/.

alxhub · 2018-02-26T19:25:35Z

After discussing with @mhevery, we're agreed that inject() should be exported publicly. It's intended to replace deps: [...] as the way of specify dependencies to factory functions, and has the major advantage that its call sites are monomorphic.

Needs re-review given change.

mary-poppins · 2018-02-26T21:06:29Z

You can preview c86fb35 at https://pr22389-c86fb35.ngbuilds.io/.

IgorMinar · 2018-02-27T02:16:20Z

packages/core/src/di/injector.ts

+ * Injects a token from the currently active injector.
+ *
+ * This function must be used in the context of a factory function such as one defined for an
+ * `InjectionToken`, and will throw an error if not called from such a context.


most people will not use InjectionToken to interact with this. Maybe expand this to say "one defined via @Injectable factory or InjectionToken" and link both the the respective api pages.

+1 for documentation with example

@Injectable does not yet have factory (you could use inject in useFactory but that's not a pattern that we've decided to teach yet).

I've added an example that shows how to use inject within an InjectionToken function.

I'll communicate this public API change to the docs team working on tree-shakeable providers as well, and they can document inject in their new material.

IgorMinar · 2018-02-27T02:18:29Z

packages/compiler-cli/integrationtest/bazel/injectable_def/app/src/dep.ts

@@ -0,0 +1,41 @@
+/**


since you are proposing to make inject a public api, you should also make use of it in this test. currently all the testing is done indirectly by relying on the generating code containing "inject" call.

I've added it to the token test.

inject() supports the ngInjectableDef-based configuration of the injector (otherwise known as tree-shakeable services). It was missing from the exported API of @angular/core, this PR adds it. The test added here is correct in theory, but may pass accidentally due to the decorator side-effect replacing the inject() call at runtime. An upcoming compiler PR will strip reified decorators from the output entirely. Fixes angular#22388 PR Close angular#22389

angular-automatic-lock-bot · 2019-09-13T09:22:37Z

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

googlebot added the cla: yes label Feb 22, 2018

IgorMinar suggested changes Feb 23, 2018

View reviewed changes

IgorMinar previously requested changes Feb 23, 2018

View reviewed changes

alxhub force-pushed the export-inject branch from a25dc34 to de32278 Compare February 23, 2018 00:27

alxhub force-pushed the export-inject branch from de32278 to 50a9b9c Compare February 23, 2018 17:06

alxhub force-pushed the export-inject branch 2 times, most recently from c0b1722 to abc6001 Compare February 23, 2018 17:22

alxhub added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Feb 23, 2018

IgorMinar previously approved these changes Feb 23, 2018

View reviewed changes

alxhub force-pushed the export-inject branch 2 times, most recently from 6aac2aa to 4d3931e Compare February 26, 2018 16:54

alxhub force-pushed the export-inject branch from 4d3931e to ef54180 Compare February 26, 2018 19:15

alxhub force-pushed the export-inject branch from ef54180 to c86fb35 Compare February 26, 2018 20:50

alexeagle added the area: core Issues related to the framework runtime label Feb 26, 2018

IgorMinar reviewed Feb 27, 2018

View reviewed changes

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): export inject() from @angular/core #22389

fix(core): export inject() from @angular/core #22389

alxhub commented Feb 22, 2018

mary-poppins commented Feb 22, 2018

IgorMinar Feb 23, 2018

alxhub Feb 23, 2018

IgorMinar Feb 23, 2018

alxhub Feb 23, 2018 •

edited

IgorMinar Feb 23, 2018

alxhub Feb 23, 2018

IgorMinar Feb 23, 2018

mary-poppins commented Feb 23, 2018

IgorMinar commented Feb 23, 2018 via email

mary-poppins commented Feb 23, 2018

ngbot bot commented Feb 23, 2018

IgorMinar left a comment

mary-poppins commented Feb 23, 2018

mary-poppins commented Feb 26, 2018

mary-poppins commented Feb 26, 2018

mary-poppins commented Feb 26, 2018

alxhub commented Feb 26, 2018

mary-poppins commented Feb 26, 2018

IgorMinar Feb 27, 2018

mhevery Feb 27, 2018

alxhub Feb 27, 2018

IgorMinar Feb 27, 2018

alxhub Feb 27, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

fix(core): export inject() from @angular/core #22389

fix(core): export inject() from @angular/core #22389

Conversation

alxhub commented Feb 22, 2018

mary-poppins commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alxhub Feb 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Feb 23, 2018

IgorMinar commented Feb 23, 2018 via email

mary-poppins commented Feb 23, 2018

ngbot bot commented Feb 23, 2018

IgorMinar left a comment

Choose a reason for hiding this comment

mary-poppins commented Feb 23, 2018

mary-poppins commented Feb 26, 2018

mary-poppins commented Feb 26, 2018

mary-poppins commented Feb 26, 2018

alxhub commented Feb 26, 2018

mary-poppins commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 13, 2019

alxhub Feb 23, 2018 •

edited