`typeForAttribute` option overriding causing `pluralizeType` not to have an effect

[jamesdixon]

@ShadowManu @chamini2 I just upgraded to the beta and am running into a regression where it seems that the options I'm passing directly to the serializer are no longer working.

Specifically, I pass in the pluralizeType and keyForAttribute options. After some investigation, it appears that the addition of the relationTypes attribute automatically creates a typeForAttribute option which is passed to the serializer.

I'm not sure I see the point of the relationTypes option when this functionality could already be had by just passing in typeForAttribute as a serializer option.

Thoughts?

[jamesdixon]

Just as a follow up, I'm of the opinion that we should not be adding a typeForAttribute option and leave it up to the user.

[chamini2]

@jamesdixon, I did not think about someone passing a custom typeForAttributes or pluralizeType options to the serializer! Definitely a bug. I'll address this tomorrow morning and let you know.

About relationTypes not being an option, the idea would be to give our own API to instead of requiring that people learn both ours and the serializer's, so it seems logical for me to have it as an option (fixing the issue you found). And another observation behind this is that since we're the ones doing the pluralization in the links, it makes sense that we take care of the whole pluralization, types included; not do half pluralization us in the links and half the serializer with the types.

[jamesdixon]

@chamini2 thanks for the explanation!

About relationTypes not being an option, the idea would be to give our own API to instead of requiring that people learn both ours and the serializer's, so it seems logical for me to have it as an option (fixing the issue you found).

Understood. Learning two APIs doesn't make sense. However, I would argue that it doesn't make sense to create our own custom API and then map our own named option only to pass the serializer a differently named option. In all honesty, the option relationTypes wasn't really all that intuitive when I first looked at it. I had to re-read the description a few times in order to understand what it was doing only to see that it was creating a typeForAttribute option. That said, wouldn't it just make more sense to document the underlying serializer API rather than creating completely new options?

Thanks for all of your hard work!

[chamini2]

@jamesdixon, fixed the bug!

I'll leave this issue open for discussion or would you prefer to open a new one?

[chamini2]

Understood. Learning two APIs doesn't make sense. However, I would argue that it doesn't make sense to create our own custom API and then map our own named option only to pass the serializer a differently named option. In all honesty, the option relationTypes wasn't really all that intuitive when I first looked at it. I had to re-read the description a few times in order to understand what it was doing only to see that it was creating a typeForAttribute option. That said, wouldn't it just make more sense to document the underlying serializer API rather than creating completely new options?

Well, documenting the underlying API means we would depend on the API decisions of the serializer, implementing our own means we could always adapt to the new API and keep ours. It also seems like an important enough feature (see #30) to give a native solution instead of sending users to the underlying tool's documentation.

Another problem I see in using typeForAttribute is that it is a function with signature (attr, data) => type, and data is the object we created for the serializer from the model; it's something very dependent of our implementation, our users don't know what to expect for that argument.

About the other comments you made:

• The description in the README sure could use a rewrite
• Maybe we could change the name to typeForRelation (hinting the similitude to the serializer typeForAttribute)

[jamesdixon]

@chamini2 these are all good points. I think with a documentation update and some clearer options, this makes more sense. However, whether we like it or not, we are at the mercy of the underlying serializer options 😄

I'm going to close this and open another issue for the API discussion.

Thank you!