scoutforpets / jsonapi-mapper

JSON API-Compliant Serialization for your Node ORM
The Unlicense
42 stars 24 forks source link

Adds ability to specify specific relations to be included #68

Closed jamesdixon closed 8 years ago

jamesdixon commented 8 years ago

@chamini2 as you suggested previously, this PR allows for specific relations to be included. I already ran into a case for this, hence the quick addition 👍

chamini2 commented 8 years ago

Hey, I don't think this is the best API to achieve this.

I was thinking maybe then to do:

relations : false // no relations, none included
relations : true // all relations, all included
relations : { included: true } // all relations, all included
relations : { included: false } // all relations, none included
relations : ['a', 'b'] // relations and included: 'a' 'b'
relations : { 'a': true, 'b': false } // relations: 'a' 'b', included: 'a'

So, dropping entirely the fields option?

I don't know if it's the best, but it limits problems in input. With the proposed API by this PR think of this case:

relations : { fields: ['a', 'b'], included: ['a', 'c'] }

EDIT, the one with { 'a': true, 'b': false} doesn't work if there's a relation called included... so maybe:

relations : false // no relations, none included
relations : true // all relations, all included
relations : { included: true } // all relations, all included
relations : { included: false } // all relations, none included
relations : ['a', 'b'] // relations and included: 'a' 'b'
relations : { included: {a: true, b: false} } // relations: 'a' 'b', included: 'a'
jamesdixon commented 8 years ago

Hey. I appreciate the suggestion, but I believe the API you proposed is too complex. It just doesn't read well and ultimately seems more of a pain to implement.

However, I do agree that the case you mentioned is problematic. I think we could easily difference those arrays to make it work. Essentially, if fields exists and included is an array, we difference on those to get the fields that can actually be included.

What do you think?

jamesdixon commented 8 years ago
relations : { included: {a: true, b: false} } // relations: 'a' 'b', included: 'a'

You still run into the original issue of not being able to specify the relations to serialize, correct?

For example, without the fields param, the above only allows you to serialize all relations and selectively include or exclude them from the payload.

chamini2 commented 8 years ago

For example, without the fields param, the above only allows you to serialize all relations and selectively include or exclude them from the payload.

I was thinking that the properties of that object would be the ones to add in relations and just include the true ones in the included array. But I see your point in it being complex.

However, I do agree that the case you mentioned is problematic. I think we could easily difference those arrays to make it work. Essentially, if fields exists and included is an array, we difference on those to get the fields that can actually be included.

I guess this works, I don't love it but seems easier to understand.

jamesdixon commented 8 years ago

@chamini2 see the changes I just pushed.

I guess this works, I don't love it but seems easier to understand.

It seems like a valid solution; not sure how we'd make it much better without making the API over-complex. What don't you love about it? ❤️ 😄

jamesdixon commented 8 years ago

Ultimately, the solution I provided is just eliminating the ability for a user to make an error. I believe we're going to encounter that regardless of what the API looks like.

jamesdixon commented 8 years ago

@chamini2 just added the test from the other branch and replaced some with includes in the relationsAllowed block.

chamini2 commented 8 years ago

Hey @jamesdixon, I opened a PR to this branch in your repo to change a bit the checks for options passed. I figured it would be easier to change it myself.

jamesdixon commented 8 years ago

Ok, changes are fine by me.

jamesdixon commented 8 years ago

Thanks for your help!