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

Support mixed types in HasMany relationship #215

Closed
hpawe01 opened this issue Apr 9, 2019 · 4 comments
Closed

Support mixed types in HasMany relationship #215

hpawe01 opened this issue Apr 9, 2019 · 4 comments

Comments

@hpawe01
Copy link
Contributor

hpawe01 commented Apr 9, 2019

A HasMany relationship (e.g. books: Book[]) does not allow mixed types (e.g. CrimeBook, NovelBook), even if they extend the same expected HasMany type (Book).

Problem
In this line, the type of the first element in the relationship is checked against all other types.

const relationshipType: string = isJsonApiModel ? objects[0].modelConfig.type : '';

Example
I have

@HasMany()
books: Book[];

and the models

@JsonApiModelConfig({
  type: 'crimeBooks',
  modelEndpointUrl: 'books'
})
export class CrimeBook extends Book {...}

@JsonApiModelConfig({
  type: 'novelBooks',
  modelEndpointUrl: 'books'
})
export class NovelBook extends Book {...}

Possible solution 1
Collect modelEndpointUrl or type of all relationship items and check if there is only one unique. This way, even if the type of the items differ (crimeBooks or novelBooks), if the modelEndpointUrl is the same (books), all should be fine.

Possible solution 2
Check, if all relationship items are instances of the model, that the HasMany attribute expects (Book).

I will create a pull request for solution 1, but I am not sure if solution 2 would be more clean (Although only type and modelEndpointUrl are relevant, when talking to the server, and not the Typescript type of the HasMany attribute).

@hpawe01
Copy link
Contributor Author

hpawe01 commented Apr 9, 2019

I think solution 1 would also solve a problem, that would occur if we use polymorphic relationships as mentions in issue #201

@hpawe01
Copy link
Contributor Author

hpawe01 commented May 20, 2019

Regarding solution 2: To realize this, TypeScript needs to expose the array element type (see microsoft/TypeScript#7169, e.g. Book for Book[]), so that we can check, that all items are instances of this array element type.

@safo6m
Copy link
Collaborator

safo6m commented Jul 18, 2019

Correct, the solution 2 is technically the correct one, but for now we can go with the solution 1 due to its simplicity.

@hpawe01
Copy link
Contributor Author

hpawe01 commented Aug 5, 2019

Closing because #216 was merged.

@hpawe01 hpawe01 closed this as completed Aug 5, 2019
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