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

Avoid core-js dependency #314

Closed
wtho opened this issue Oct 1, 2019 · 2 comments · Fixed by #315
Closed

Avoid core-js dependency #314

wtho opened this issue Oct 1, 2019 · 2 comments · Fixed by #315
Labels
dependencies Pull requests that update a dependency file

Comments

@wtho
Copy link
Collaborator

wtho commented Oct 1, 2019

Supersedes #288 partially.

Problem

Angular dropped core-js as dependency, but we still use it for polyfilling reflection of TS types.

Note: I will give a lot of by research background here to have it documented somewhere.

Reflection in TS/JS and Angular

Reflection usually gives developer access to language features that are not available by default. In the context of TS/JS this is e. g. the types that are removed at compile time. The reflection API does preserve them and store them on JS objects, so they can be accessed at runtime in the JS code.

Angular uses this for its dependency injection, in constructors. Currently there are several ways to achieve this:

TS emitDecoratorMetadata and reflect-metadata proposal

This is the default way that was used for a long while by Angular now. When using emitDecoratorMetadata: true in tsconfig.json, TSC will emit classes with constructor parameters as follows:

@Injectable()
export class NgLocaleLocalization {
  constructor(
    @Optional() @Inject(TOKEN_1) protected locale: string,
    private _renderer: Renderer2
  ) { }
}

compiles to

var NgLocaleLocalization = /** @class */ (function (_super) {
  function NgLocaleLocalization(locale, _renderer2) { }
  NgLocaleLocalization = __decorate([
    core.Injectable(),
    __param(0, core.Optional()), __param(0, core.Inject(TOKEN_1)),
    __metadata("design:paramtypes", [String, core.Renderer2])
  ], NgLocaleLocalization);
  return NgLocaleLocalization;
}(NgLocalization));

Note that the __metadata part will only be compiled, if emitDecoratorMetadata: true is set in tsconfig.json.

__metadata will call an Angular function to let it deal with the types in some way, whereas __params will just put the parameter decorators on the function through TS directly. Angular will then call the reflection polyfill using global['Reflect'] if available and let it deal with storing the param types in some way. If core-js or reflect-metadata is not available they will be lost, and DI will not work.

TS ASTTransformer to avoid reflect metadata dependency

Angular has since changed it's approach and created an ast transformer that puts the constructor parameters on the class as static fields, on the property ctorParameters (inspired by tsickle). See the tests of this transformer to see examples.

Options to drop core-js I identified

  1. We can take the ast transformer and add it to our compilation. This would solve the problem for all classes compiled from TS, as they all can then provide the ctorParameters. emitDecoratorMetadata: true can then be removed again and we are closer to the Angular tsconfig settings again.
    Unfortunately this is not a complete solution, as not all classes and modules are provided in TS. In Angulars bundles there are precompiled classes like TestBed, CommonModule and more, that do not have ctorParameters and therefore require a reflect-metadata polyfill.
    Furthermore the compile environment of the Angular CLI is different from the one of ts-jest, so we cannot just copy & paste the transformer, but have to make some adjustments. This would this would mean we add around 300 LOC to our codebase that we have to keep testing, adjusting and managing.
  2. Use the reflect-metadata proposal library.
    This library was created for the initial proposal. core-js just reimplemented its functionality. While it is much smaller than core-js (just 332K), it would still be an additional library.
  3. Provide a minimal reflect metadata library on our own
    We provide a minimal implementation of the two used methods Reflect.metadata and Reflect.getOwnMetadata to store and retrieve the constructor parameter information. While this would not be a full functional reflect metadata library, it would still be sufficient to work with Angular. This would still require emitDecoratorMetadata: true, but through this we can drop the 6.8M dependency core-js.
    A minimal working example would look like this:
globalThis["Reflect"] = {
  metadata: (mtKey, mtValue) => (target, _propertyKey) => {
    if (mtKey === "design:paramtypes")
      target.ctorParametersJPA = mtValue
  },
  getOwnMetadata: (mtKey, target) => {
    if (mtKey === 'design:paramtypes')
      return target.ctorParametersJPA
  }
}
  1. Keep core-js.
    Yes, it is a big dependency, but it only loads the parts required, so it is not as slow as its size suggests. Plus it is still required by some deeper dependencies as well, so if npm/yarn do its job well there should not be additional space used. Still it might be dropped by those as well in some future, and we might not want jest-preset-angular to be the reason to install it.
@wtho
Copy link
Collaborator Author

wtho commented Oct 1, 2019

There might be other options, but these were the ones I identified to be viable.

I would suggest to use third option, as it is basically 10 LOC instead of a 6.8M dependency. This makes it also maintainable and fast. If you are ok with this I would open a PR.

@wtho wtho added the dependencies Pull requests that update a dependency file label Oct 1, 2019
@ahnpnl
Copy link
Collaborator

ahnpnl commented Oct 1, 2019

I also agreed with 3rd option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants