data icon indicating copy to clipboard operation
data copied to clipboard

Third param of `JSONAPISerializer.shouldSerializeHasMany` documented as relationType instead of RelationshipMeta

Open azhiv opened this issue 3 years ago • 3 comments

As a part of https://github.com/emberjs/data/pull/4279 the method shouldSerializeHasMany on JSON API Serializer was made public. However, the documented signature doesn't correctly reflect the third relation parameter and its type. Here's a link to the code where you can see that the third parameter is relation (not relationType as per docs):

    @public
    @param {Snapshot} snapshot
    @param {String} key
    @param {String} relationshipType
    @return {boolean} true if the hasMany relationship should be serialized
  */
  shouldSerializeHasMany(snapshot, key, relationship) {

which has in fact a type called RelationshipMeta described here.

I couldn't find a definition of RelationshipMeta in the docs, but since the documentation is misleading - what would be the correct way to address this issue? As a drive-by change the description for Model.inverseFor could also be updated along with:

  • types for inverseFor as the return type should also be RelationshipMeta
  • the test for shouldSerializeHasMany because it doesn't look correct:
var snapshot = post._createSnapshot();
var relationship = snapshot.record.relationshipFor('comments');
var key = relationship.key;

var shouldSerialize = env.store.serializerFor("post").shouldSerializeHasMany(snapshot, relationship, key);

The last call should be made with parameters slightly reordered (see above):

shouldSerializeHasMany(snapshot, key, relationship);

azhiv avatar May 26 '22 16:05 azhiv

Hi thanks for realizing this. I'm not sure what the DefinitelyTyped types say (they are often wrong) but in terms of EmberData this is what we would refer to as a RelationshipSchema. The current interface is below.

interface RelationshipSchema {
  kind: 'belongsTo' | 'hasMany';
  type: string; // the related model type

  // The property key for this relationship
  name: string;

  options: {
    async: boolean;
    polymorphic?: boolean;
    inverse?: string | null; // property key on the related type (if any)
    [key: string]: unknown;
  };

  parentModelName: string;
}

runspired avatar May 26 '22 21:05 runspired

What is the correct way of moving forward here? Should we make this interface public and update js-doc description for shouldSerializeHasMany and inverseFor? It would be great to do so because this will allow us to reference the type from our code (right now I have to partly redeclare the interface). DefinitelyTyped could also be updated in this case.

azhiv avatar May 30 '22 11:05 azhiv

@azhiv

Should we make this interface public and update js-doc description for shouldSerializeHasMany and inverseFor?

No, ember-data does not publish types. We can describe this in the js-docs for shouldSerializeHasMany and inverseFor though.

DefinitelyTyped could also be updated in this case.

Yep! absolutely help that project please!

runspired avatar Jul 15 '22 21:07 runspired