feat(core): Create StaticInjector which does not depend on Reflect polyfill by mhevery · Pull Request #18496 · angular/angular

mhevery · 2017-08-02T21:48:59Z

No description provided.

devoto13 · 2017-08-03T14:25:05Z

Is duplicate signature intended?

Yes, see https://www.typescriptlang.org/docs/handbook/functions.html#overloads

tbosch · 2017-08-04T18:15:26Z

+ *
+ * @stable
+ */
+export type Provider =


Provider is also used in our decorators (e.g. @NgModule). It seems wrong that it is now defined in a file called reflective_injector_interfaces. I think it should be defined in e.g. di/metadata.ts and then this file can reexport it if we really need this file.

moved back to provider.ts

tbosch · 2017-08-04T18:17:19Z

+  let value: any = EMPTY;
+  let useNew: boolean = false;
+  let provide = resolveForwardRef(provider.provide);
+  if (USE_VALUE in provider) {


Why use in here? For all the cases we can just check for e.g. .useFactory !== undefined, and the fall through case is useValue...

This would both be faster and remove getClosureSafeProperty

I can't use .useValue since the value could be undefined. So since I started using in with useValue, I am just being consistent.

changed, only USE_VALUE is special

tbosch · 2017-08-04T18:24:15Z

+}
+
+interface Records {
+  [key: string]: Record;


Why use an object hash and not a Map directly?

See: https://stackoverflow.com/questions/10258071/class-object-vs-hashmap

Summary: faster, smaller, no polyfill

changed to map

tbosch · 2017-08-04T18:25:20Z

+
+let idCounter = 0;
+
+function getId(obj: any): string {


I think using a Map is simpler, and shouldn't be slower AFAIK.

tbosch · 2017-08-04T18:27:18Z

+        }
+      }
+      if (useNew) {
+        obj = Object.create(fn.prototype);


This won't work if users run in real ES6 mode.
Consider doing a obj = new fn(...deps).

good catch, fixed

tbosch · 2017-08-04T18:31:50Z

+const NG_TOKEN_PATH = 'ngTokenPath';
+const NG_TEMP_TOKEN_PATH = 'ngTempTokenPath';
+const ID_EXPANDO = '__symbol_angular_id_' + new Date().getTime() + '__';
+const OPT_OPTIONAL = 1;


Consider using an enum for the bitmask here and add the suffix Flags (which is what we do everywhere else):

enum OptionFlags { Optional = 1 << 0, CheckSelf = 1 << 1, CheckParent = 1 << 2, }

tbosch · 2017-08-04T18:34:11Z

+    if (!(e instanceof Error)) {
+      e = new Error(e);
+    }
+    const path: any[] = e[NG_TEMP_TOKEN_PATH] = e[NG_TEMP_TOKEN_PATH] || [];


Consider passing the tokenPathOnError directly to tryResolveToken, so you don't need to patch it unto the error (as it is a private field anyways).

I don't want to have the cost of tokenPath only in the case of an error. Hence it is patched on Error

I mean you can just create it as an empty array, and only fill it in case of an error in the various catch clauses. This way, the cost is only to create the empty array, but not to fill it.

tbosch · 2017-08-04T18:34:51Z

+      if (depRecords.length) {
+        deps = [];
+        for (let i = 0; i < depRecords.length; i++) {
+          const depRecord: DependencyRecord = depRecords[i], options = depRecord.options,


Maybe have separate const statements to clear up the formatting?

tbosch · 2017-08-04T18:36:09Z

+    } catch (e) {
+      const tokenPath: any[] = e[NG_TEMP_TOKEN_PATH];
+      let message: string = e.message;
+      message = message && message.charAt(0) === NO_NEW_LINE ? message.substr(1) : '\n' + message;


Consider adding the logic of adding a newLine or not to staticError

tbosch · 2017-08-04T18:37:54Z

+      let message: string = e.message;
+      message = message && message.charAt(0) === NO_NEW_LINE ? message.substr(1) : '\n' + message;
+      const error = staticError(message, tokenPath);
+      e.message = error.message;


Did you check if this prints nicely in the browser console? AFAIK it does not.
Another solution would be to throw 2 errors: 1 for flow control where it is now, and another one here with the proper message passed to new Error(...)

Yes, it looks good in console.

tbosch · 2017-08-04T18:42:13Z


+    describe('StaticClassProvider', () => {
+      it('works', () => {
+        // #docregion StaticClassProvider


I think this is missing a guide that refers to these new #docregions...

Not sure I understand. It is present.

Ah, never mind, I remembered incorrectly how the guides work.

tbosch

Looks good in general, needs cleanup.

…lyfill.

vicb · 2017-08-07T18:26:50Z

blocked on tap/164481065

mhevery · 2017-08-07T20:10:58Z

@vicb here is the fix http://cl/164500104

IgorMinar · 2017-08-07T18:49:00Z

This change allows ReflectiveInjector to be tree shaken resulting in not needed Reflect polyfil and smaller bundles. Code savings for HelloWorld using Closure: Reflective: bundle.js: 105,864(34,190 gzip) Static: bundle.js: 154,889(33,555 gzip) 645( 2%) BREAKING CHANGE: `platformXXXX()` no longer accepts providers which depend on reflection. Specifically the method signature when from `Provider[]` to `StaticProvider[]`. Example: Before: ``` [ MyClass, {provide: ClassA, useClass: SubClassA} ] ``` After: ``` [ {provide: MyClass, deps: [Dep1,...]}, {provide: ClassA, useClass: SubClassA, deps: [Dep1,...]} ] ``` NOTE: This only applies to platform creation and providers for the JIT compiler. It does not apply to `@Compotent` or `@NgModule` provides declarations. Benchpress note: Previously Benchpress also supported reflective provides, which now require static providers. DEPRECATION: - `ReflectiveInjector` is now deprecated as it will be remove. Use `Injector.create` as a replacement.

follow-up for angular#18496

trotyl · 2017-08-08T02:23:17Z

Would #13820 be affected by this? Seems StaticInjector does not require @Injectable() anymore.

sod · 2017-08-08T13:31:39Z

@Injectable() is just a trick to get typescript to export the typings from the constructor, thanks to emitDecoratorMetadata setting in tsconfig.json. It'll most likley stay.

trotyl · 2017-08-08T13:37:56Z

@sod #13820 is about requiring @Injectable() when there is no dependency, where metadata is not needed here.

sod · 2017-08-08T14:41:37Z

Commit message fcadbf4 reads like you are indeed right. They deprecated or even got rid of the reflective resolver altogether.

follow-up for #18496

This change allows ReflectiveInjector to be tree shaken resulting in not needed Reflect polyfil and smaller bundles. Code savings for HelloWorld using Closure: Reflective: bundle.js: 105,864(34,190 gzip) Static: bundle.js: 154,889(33,555 gzip) 645( 2%) BREAKING CHANGE: `platformXXXX()` no longer accepts providers which depend on reflection. Specifically the method signature when from `Provider[]` to `StaticProvider[]`. Example: Before: ``` [ MyClass, {provide: ClassA, useClass: SubClassA} ] ``` After: ``` [ {provide: MyClass, deps: [Dep1,...]}, {provide: ClassA, useClass: SubClassA, deps: [Dep1,...]} ] ``` NOTE: This only applies to platform creation and providers for the JIT compiler. It does not apply to `@Compotent` or `@NgModule` provides declarations. Benchpress note: Previously Benchpress also supported reflective provides, which now require static providers. DEPRECATION: - `ReflectiveInjector` is now deprecated as it will be remove. Use `Injector.create` as a replacement. closes angular#18496

follow-up for angular#18496

This change allows ReflectiveInjector to be tree shaken resulting in not needed Reflect polyfil and smaller bundles. Code savings for HelloWorld using Closure: Reflective: bundle.js: 105,864(34,190 gzip) Static: bundle.js: 154,889(33,555 gzip) 645( 2%) BREAKING CHANGE: `platformXXXX()` no longer accepts providers which depend on reflection. Specifically the method signature when from `Provider[]` to `StaticProvider[]`. Example: Before: ``` [ MyClass, {provide: ClassA, useClass: SubClassA} ] ``` After: ``` [ {provide: MyClass, deps: [Dep1,...]}, {provide: ClassA, useClass: SubClassA, deps: [Dep1,...]} ] ``` NOTE: This only applies to platform creation and providers for the JIT compiler. It does not apply to `@Compotent` or `@NgModule` provides declarations. Benchpress note: Previously Benchpress also supported reflective provides, which now require static providers. DEPRECATION: - `ReflectiveInjector` is now deprecated as it will be remove. Use `Injector.create` as a replacement. closes angular#18496

follow-up for angular#18496

This change allows ReflectiveInjector to be tree shaken resulting in not needed Reflect polyfil and smaller bundles. Code savings for HelloWorld using Closure: Reflective: bundle.js: 105,864(34,190 gzip) Static: bundle.js: 154,889(33,555 gzip) 645( 2%) BREAKING CHANGE: `platformXXXX()` no longer accepts providers which depend on reflection. Specifically the method signature when from `Provider[]` to `StaticProvider[]`. Example: Before: ``` [ MyClass, {provide: ClassA, useClass: SubClassA} ] ``` After: ``` [ {provide: MyClass, deps: [Dep1,...]}, {provide: ClassA, useClass: SubClassA, deps: [Dep1,...]} ] ``` NOTE: This only applies to platform creation and providers for the JIT compiler. It does not apply to `@Compotent` or `@NgModule` provides declarations. Benchpress note: Previously Benchpress also supported reflective provides, which now require static providers. DEPRECATION: - `ReflectiveInjector` is now deprecated as it will be remove. Use `Injector.create` as a replacement. closes angular#18496

follow-up for angular#18496

angular-automatic-lock-bot · 2019-09-12T06:20:52Z

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 Aug 2, 2017

mhevery force-pushed the no_reflective_injector branch from fa8f677 to 750f4b7 Compare August 2, 2017 22:39

vicb added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 3, 2017

devoto13 reviewed Aug 3, 2017

View reviewed changes

mhevery force-pushed the no_reflective_injector branch 3 times, most recently from 97fc905 to 5e07e15 Compare August 3, 2017 20:46

tbosch added the feature Label used to distinguish feature request from other issues label Aug 3, 2017

mhevery force-pushed the no_reflective_injector branch from 5e07e15 to 4158b94 Compare August 3, 2017 23:20

mhevery requested a review from tbosch August 3, 2017 23:20

mhevery force-pushed the no_reflective_injector branch 2 times, most recently from 85e97a0 to 763a3a9 Compare August 4, 2017 17:47

tbosch reviewed Aug 4, 2017

View reviewed changes

tbosch suggested changes Aug 4, 2017

View reviewed changes

mhevery force-pushed the no_reflective_injector branch 3 times, most recently from 5c8714d to fd8649c Compare August 4, 2017 22:27

feat(core): Create StaticInjector which does not depend on Reflect po…

006bd18

…lyfill.

mhevery force-pushed the no_reflective_injector branch from fd8649c to 00f50cb Compare August 4, 2017 23:26

mhevery force-pushed the no_reflective_injector branch 2 times, most recently from 86d6f8d to 6fc47e6 Compare August 7, 2017 16:08

mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 7, 2017

vicb added the state: blocked label Aug 7, 2017

IgorMinar approved these changes Aug 7, 2017

View reviewed changes

Comment thread packages/core/src/change_detection/differs/keyvalue_differs.ts Outdated

Copy link
Copy Markdown

Contributor

IgorMinar Aug 7, 2017

Choose a reason for hiding this comment

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

~~?

mhevery force-pushed the no_reflective_injector branch from 6fc47e6 to 5db475c Compare August 7, 2017 21:55

mhevery removed the state: blocked label Aug 7, 2017

vicb closed this in fcadbf4 Aug 7, 2017

vicb added a commit to vicb/angular that referenced this pull request Aug 7, 2017

fix(core): fix platform-browser-dynamic

a1f08bb

follow-up for angular#18496

vicb mentioned this pull request Aug 7, 2017

fix(core): fix platform-browser-dynamic #18576

Merged

vicb added a commit that referenced this pull request Aug 8, 2017

fix(core): fix platform-browser-dynamic (#18576)

f0a5501

follow-up for #18496

trotyl mentioned this pull request Aug 9, 2017

Deprecate Injectable decorator factory #18604

Closed

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

fix(core): fix platform-browser-dynamic (angular#18576)

e813b88

follow-up for angular#18496

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 26, 2017

fix(core): fix platform-browser-dynamic (angular#18576)

63edbc4

follow-up for angular#18496

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

fix(core): fix platform-browser-dynamic (angular#18576)

ca85e52

follow-up for angular#18496

trotyl mentioned this pull request Jul 21, 2018

Perf: Make platforms not use the ReflectiveInjector #11646

Closed

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

Conversation

mhevery commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbosch left a comment

Choose a reason for hiding this comment

Uh oh!

vicb commented Aug 7, 2017

Uh oh!

mhevery commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trotyl commented Aug 8, 2017

Uh oh!

sod commented Aug 8, 2017

Uh oh!

trotyl commented Aug 8, 2017

Uh oh!

sod commented Aug 8, 2017

Uh oh!

angular-automatic-lock-bot Bot commented Sep 12, 2019

Uh oh!