From 641c671bac89ba87bb77a0c99fd0b955baa5f928 Mon Sep 17 00:00:00 2001
From: Pete Bacon Darwin <pete@bacondarwin.com>
Date: Mon, 11 Nov 2019 15:01:58 +0000
Subject: [PATCH] =?UTF-8?q?fix(core):=20support=20`ngInjectableDef`=20on?=
 =?UTF-8?q?=20types=20with=20inherited=20`=C9=B5prov`=20(#33732)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The `ngInjectableDef` property was renamed to `ɵprov`, but core must
still support both because there are published libraries that use the
older term.

We are only interested in such properties that are defined directly on
the type being injected, not on base classes. So there is a check that
the defintion is specifically for the given type.

Previously if you tried to inject a class that had `ngInjectableDef` but
also inherited `ɵprov` then the check would fail on the `ɵprov` property
and never even try the `ngInjectableDef` property resulting in a failed
injection.

This commit fixes this by attempting to find each of the properties
independently.

Fixes https://github.com/angular/ngcc-validation/pull/526

PR Close #33732
---
 .../bazel/injectable_def/app/test/app_spec.ts | 26 +++++++++++++++++++
 packages/core/src/di/interface/defs.ts        | 24 +++++++++++------
 .../injection/bundle.golden_symbols.json      |  3 +++
 .../bundling/todo/bundle.golden_symbols.json  |  3 +++
 4 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/packages/compiler-cli/integrationtest/bazel/injectable_def/app/test/app_spec.ts b/packages/compiler-cli/integrationtest/bazel/injectable_def/app/test/app_spec.ts
index cf01fe5278..793a27c4d6 100644
--- a/packages/compiler-cli/integrationtest/bazel/injectable_def/app/test/app_spec.ts
+++ b/packages/compiler-cli/integrationtest/bazel/injectable_def/app/test/app_spec.ts
@@ -160,6 +160,32 @@ describe('ngInjectableDef Bazel Integration', () => {
     expect(() => TestBed.inject(ChildService).value).toThrowError(/ChildService/);
   });
 
+  it('uses legacy `ngInjectable` property even if it inherits from a class that has `ɵprov` property',
+     () => {
+       @Injectable({
+         providedIn: 'root',
+         useValue: new ParentService('parent'),
+       })
+       class ParentService {
+         constructor(public value: string) {}
+       }
+
+       // ChildServices exteds ParentService but does not have @Injectable
+       class ChildService extends ParentService {
+         constructor(value: string) { super(value); }
+         static ngInjectableDef = {
+           providedIn: 'root',
+           factory: () => new ChildService('child'),
+           token: ChildService,
+         };
+       }
+
+       TestBed.configureTestingModule({});
+       // We are asserting that system throws an error, rather than taking the inherited
+       // annotation.
+       expect(TestBed.inject(ChildService).value).toEqual('child');
+     });
+
   it('NgModule injector understands requests for INJECTABLE', () => {
     TestBed.configureTestingModule({
       providers: [{provide: 'foo', useValue: 'bar'}],
diff --git a/packages/core/src/di/interface/defs.ts b/packages/core/src/di/interface/defs.ts
index ac99be13a0..7a6662371b 100644
--- a/packages/core/src/di/interface/defs.ts
+++ b/packages/core/src/di/interface/defs.ts
@@ -190,14 +190,22 @@ export function ɵɵdefineInjector(options: {factory: () => any, providers?: any
  * @param type A type which may have its own (non-inherited) `ɵprov`.
  */
 export function getInjectableDef<T>(type: any): ɵɵInjectableDef<T>|null {
-  const def = (type[NG_PROV_DEF] || type[NG_INJECTABLE_DEF]) as ɵɵInjectableDef<T>;
-  // The definition read above may come from a base class. `hasOwnProperty` is not sufficient to
-  // distinguish this case, as in older browsers (e.g. IE10) static property inheritance is
-  // implemented by copying the properties.
-  //
-  // Instead, the ɵprov's token is compared to the type, and if they don't match then the
-  // property was not defined directly on the type itself, and was likely inherited. The definition
-  // is only returned if the type matches the def.token.
+  return getOwnDefinition(type, type[NG_PROV_DEF]) ||
+      getOwnDefinition(type, type[NG_INJECTABLE_DEF]);
+}
+
+/**
+ * Return `def` only if it is defined directly on `type` and is not inherited from a base
+ * class of `type`.
+ *
+ * The function `Object.hasOwnProperty` is not sufficient to distinguish this case because in older
+ * browsers (e.g. IE10) static property inheritance is implemented by copying the properties.
+ *
+ * Instead, the definition's `token` is compared to the `type`, and if they don't match then the
+ * property was not defined directly on the type itself, and was likely inherited. The definition
+ * is only returned if the `type` matches the `def.token`.
+ */
+function getOwnDefinition<T>(type: any, def: ɵɵInjectableDef<T>): ɵɵInjectableDef<T>|null {
   return def && def.token === type ? def : null;
 }
 
diff --git a/packages/core/test/bundling/injection/bundle.golden_symbols.json b/packages/core/test/bundling/injection/bundle.golden_symbols.json
index 52891d4105..76e6fcdb57 100644
--- a/packages/core/test/bundling/injection/bundle.golden_symbols.json
+++ b/packages/core/test/bundling/injection/bundle.golden_symbols.json
@@ -131,6 +131,9 @@
   {
     "name": "getNullInjector"
   },
+  {
+    "name": "getOwnDefinition"
+  },
   {
     "name": "getUndecoratedInjectableFactory"
   },
diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
index cb14f93b75..de1cb1a2db 100644
--- a/packages/core/test/bundling/todo/bundle.golden_symbols.json
+++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -737,6 +737,9 @@
   {
     "name": "getOriginalError"
   },
+  {
+    "name": "getOwnDefinition"
+  },
   {
     "name": "getParentInjectorIndex"
   },