ghidoz / angular2-jsonapi

A lightweight Angular 2 adapter for JSON API
199 stars 123 forks source link

Incompatible with default Angular 8 app #236

Open piotrpalek opened 5 years ago

piotrpalek commented 5 years ago

Hey, Angular 8 targets es2015 and doesn't have emitDecoratorMetadata enabled. Which seems to make this library fundamentally incompatible with the newest version. (see also: https://github.com/angular/angular-cli/issues/15077)

I may totally misunderstand all of this (new to this ecosystem) but as I understand it, this library uses the "Metadata Reflection API" in a way which depends on emitDecoratorMetadata to be true which means we can't target es2015 right?

So as I see the only options are to either use es5 or stop using angular2-jsonapi? (correct me if I'm totally off the tracks here)

hpawe01 commented 5 years ago

Isn't that what we have polyfills.ts for? ng update removes most of the polyfills but when I add import 'core-js/proposals/reflect-metadata'; to polyfills.ts (as proposed here) all works as expected (for me).

piotrpalek commented 5 years ago

I'd need to try and make a reproducible example, but from my limited understanding the issue isn't that we can't use the Reflection API itself (that can be polyfilled as you've noted). The issue is that this library seems to use information about types from that metadata. And if emitDecoratorMetadata is set to false there is no metadata about types provided, breaking the library.

At least that was my conclusion, because I've had an issue with a PropertyConverter not working after turning off the emitDecoratorMetadata, and it broke at a place where it tried to read info about the type of the property from the Reflection API.

hpawe01 commented 5 years ago

Just tested it with a fresh Angular CLI project (v8.2.0) and experienced the same issue with the emitDecoratorMetadata option. I created a pull request to add this information to the README (#237) as all new Angular CLI projects will run into this problem.

Do you know what the "fundamental design limitation" of this option with ES2015 code is?

The "reference error" that is mentioned in the issue you linked (https://github.com/angular/angular-cli/issues/15077) was easy to handle for me. I experienced this with the example from the README: ReferenceError: Cannot access 'User' before initialization and just had to reorder my classes (although in a real project you wouldn't put multiple classes in one file, so this problem wouldn't occur).

piotrpalek commented 5 years ago

In the example from the Readme you can reorder things, but sometimes you can have structures which would result in a circular dependency, and I'm not sure it's possible to re-order at that point.

Like having class A with a relationship depending on class B, while also having class B with a relationship depending on class A, wouldn't that be impossible to define at this point? Something like:

@JsonApiModelConfig({
  type: 'posts'
})
export class Post extends JsonApiModel {
  @BelongsTo()
  comments: Comment;
}

@JsonApiModelConfig({
  type: 'comments'
})
export class Comment extends JsonApiModel {
  @BelongsTo()
  post: Post;
}

Can this be fixed somehow?

hpawe01 commented 5 years ago

Sorry, but I have no idea how to fix this. Until someone does, projects that have this problem have to use es5. Fortunately all the projects I manage don't have this problem (yet) (and when they did I always found a way to restructure them to prevent circular dependencies) and I can let the cli compile to es2015.

hpawe01 commented 5 years ago

I updated my pull request (#237) to add more detailed information about the compile targets.