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

Incompatible with default Angular 8 app #236

Open
piotrpalek opened this issue Aug 7, 2019 · 6 comments
Open

Incompatible with default Angular 8 app #236

piotrpalek opened this issue Aug 7, 2019 · 6 comments

Comments

@piotrpalek
Copy link

piotrpalek commented Aug 7, 2019

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: angular/angular-cli#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
Copy link
Contributor

hpawe01 commented Aug 8, 2019

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
Copy link
Author

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
Copy link
Contributor

hpawe01 commented Aug 8, 2019

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 (angular/angular-cli#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
Copy link
Author

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
Copy link
Contributor

hpawe01 commented Aug 8, 2019

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
Copy link
Contributor

hpawe01 commented Aug 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants