Skip to content

Conversation

@abrararshad
Copy link
Contributor

@abrararshad abrararshad commented Sep 24, 2017

Currently, we only get one type of models per HasMany field but if there are more than one type they are not being checked.

For example

-- entity:
----relationships:
--------components:
------------Type: A
------------Type: B

Whichever comes first in relationships array gets loaded by the library (in this case Type:A), not the second type (in this case Type:B)

Another loop is added in parseHasMany function of json-api.model.ts. And it also tracks typeName not to loop over again by the same type in order to avoid duplicate entries.

@coveralls
Copy link

coveralls commented Sep 25, 2017

Coverage Status

Coverage increased (+0.3%) to 87.75% when pulling 5db7390 on abrararshad:master into 17ae489 on ghidoz:master.

@abrararshad abrararshad changed the title support for multipe types per HasMany field Support for multiple types per HasMany field Sep 25, 2017
@coveralls
Copy link

coveralls commented Sep 25, 2017

Coverage Status

Coverage increased (+0.3%) to 87.75% when pulling cfb60a2 on abrararshad:master into 17ae489 on ghidoz:master.

@mortenson
Copy link

I had the same issue, and this fixed the problem for me. Thanks @abrararshad!

Copy link
Collaborator

@HennerM HennerM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, only minor things remaining.

if (relationshipModel.length > 0) {
this[metadata.propertyName] = relationshipModel;
let allModels: any = [];
let modelTypesFetched: any = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you avoid any, this should be string[].

let modelType: ModelType<this> = Reflect.getMetadata('JsonApiDatastoreConfig', this._datastore.constructor).models[typeName];
if (modelType) {
let relationshipModel: JsonApiModel[] = this.getHasManyRelationship(modelType, relationship.data, included, typeName, level);
if (relationshipModel.length > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you define allModels als var and write this as allModels = allModels.concat(relationshipModels)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to relationshipModels.

@abrararshad
Copy link
Contributor Author

Travis is throwing error at "Failed at the angular2-jsonapi@3.7.1 prepublish script 'ngc'."

Well I have ran all scripts locally build, tests, prepublish, they are all good. I will try to push again to see if it passes

@abrararshad
Copy link
Contributor Author

I was viewing the logs through a small window and could not notice this error on Travic before "Could not find a declaration file for module 'date-fns/format'" I added @types/date-fns into dev even though its readme says its not needed

@abrararshad
Copy link
Contributor Author

Just confirming. I even changed the code back to the commit when the test was successfully passed. I think it needs more eyes on this probably.

@abrararshad
Copy link
Contributor Author

Rebased the commits. Tests passed locally

@HennerM HennerM merged commit 99650fe into ghidoz:master Oct 18, 2017
@manojkukkala
Copy link

manojkukkala commented Oct 27, 2017

Hi @ghidoz can you please update latest code. It will be very helpful for our project. Thanks

@vianneyniji
Copy link

vianneyniji commented Nov 9, 2017

Nice work, just one issue for me.

If you have:
-- entity:
----relationships:
--------components:
------------Type: A
------------Type: B
------------Type: A
------------Type: B

You will loose the order A -> B -> A -> B. The output is A -> A -> B -> B

UPDATE: I fixed this issue on a fork:
vianneyniji@2f948f1

jatorresdev added a commit to jatorresdev/angular2-jsonapi that referenced this pull request Mar 14, 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

Successfully merging this pull request may close these issues.

6 participants