Skip to content
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

feat(core): Create StaticInjector which does not depend on Reflect polyfill #18496

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
8 participants
@mhevery
Copy link
Member

mhevery commented Aug 2, 2017

No description provided.

@googlebot googlebot added the cla: yes label Aug 2, 2017

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch from fa8f677 to 750f4b7 Aug 2, 2017


get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T): T;
get(token: any, notFoundValue?: any): any;
get(token: any, notFoundValue?: any): any {

This comment has been minimized.

Copy link
@devoto13

devoto13 Aug 3, 2017

Contributor

Is duplicate signature intended?

This comment has been minimized.

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch 3 times, most recently from 97fc905 to 5e07e15 Aug 3, 2017

@tbosch tbosch added the type: feature label Aug 3, 2017

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch from 5e07e15 to 4158b94 Aug 3, 2017

@mhevery mhevery requested a review from tbosch Aug 3, 2017

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch 2 times, most recently from 85e97a0 to 763a3a9 Aug 4, 2017

*
* @stable
*/
export type Provider =

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

moved back to provider.ts

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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 comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

This would both be faster and remove getClosureSafeProperty

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

changed, only USE_VALUE is special

}

interface Records {
[key: string]: Record;

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

Why use an object hash and not a Map directly?

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

changed to map


let idCounter = 0;

function getId(obj: any): string {

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

changed

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

good catch, fixed

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

Done.

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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.

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

Maybe have separate const statements to clear up the formatting?

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

done

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

Done

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;

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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(...)

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

Yes, it looks good in console.

@@ -89,6 +89,61 @@ export function main() {
});
});

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

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 4, 2017

Author Member

Not sure I understand. It is present.

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

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

@tbosch
Copy link
Member

tbosch left a comment

Looks good in general, needs cleanup.

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch 3 times, most recently from 5c8714d to fd8649c Aug 4, 2017

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch from fd8649c to 00f50cb Aug 4, 2017

@tbosch

tbosch approved these changes Aug 4, 2017

@@ -9,7 +9,7 @@
// Must be imported first, because Angular decorators throw on load.
import 'reflect-metadata';

export {InjectionToken, Injector, Provider, ReflectiveInjector} from '@angular/core';
export {InjectionToken, Injector, Provider, ReflectiveInjector, StaticProvider} from '@angular/core';

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

Why the changes in benchpress? We might have some code in G3 that relies on this supporting reflective injector...

This comment has been minimized.

Copy link
@tbosch

tbosch Aug 4, 2017

Member

Never mind, I think it is actually good to change benchpress as well.

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch 3 times, most recently from 86d6f8d to 6fc47e6 Aug 5, 2017

@vicb

This comment has been minimized.

Copy link
Contributor

vicb commented Aug 7, 2017

blocked on tap/164481065

@mhevery

This comment has been minimized.

Copy link
Member Author

mhevery commented Aug 7, 2017

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

@@ -35,7 +35,7 @@ export interface KeyValueDiffer<K, V> {
*/
diff(object: {[key: string]: V}): KeyValueChanges<string, V>;
// TODO(TS2.1): diff<KP extends string>(this: KeyValueDiffer<KP, V>, object: Record<KP, V>):
// KeyValueDiffer<KP, V>;
// KeyValueDiffer<KP, V>;~~

This comment has been minimized.

Copy link
@IgorMinar
perf: switch angular to use StaticInjector instead of ReflectiveInjector
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.

@mhevery mhevery force-pushed the mhevery:no_reflective_injector branch from 6fc47e6 to 5db475c Aug 7, 2017

@vicb vicb closed this in fcadbf4 Aug 7, 2017

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

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Aug 8, 2017

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

@sod

This comment has been minimized.

Copy link

sod commented Aug 8, 2017

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

image

@trotyl

This comment has been minimized.

Copy link
Contributor

trotyl commented Aug 8, 2017

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

@sod

This comment has been minimized.

Copy link

sod commented Aug 8, 2017

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

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

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

perf: switch angular to use StaticInjector instead of ReflectiveInjector
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

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

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

perf: switch angular to use StaticInjector instead of ReflectiveInjector
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

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

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

perf: switch angular to use StaticInjector instead of ReflectiveInjector
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.