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

Third param of JSONAPISerializer.shouldSerializeHasMany documented as relationType instead of RelationshipMeta #7996

Closed
azhiv opened this issue May 26, 2022 · 3 comments
Labels
good-first-issue 🏷️ doc This PR adds/improves/or fixes documentation

Comments

@azhiv
Copy link
Contributor

azhiv commented May 26, 2022

As a part of #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);
@runspired
Copy link
Contributor

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;
}

@azhiv
Copy link
Contributor Author

azhiv commented May 30, 2022

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.

@runspired runspired changed the title shouldSerializeHasMany is incorrectly documented Third param of shouldSerializeHasMany documented as relationType instead of RelationshipMeta Jul 15, 2022
@runspired runspired changed the title Third param of shouldSerializeHasMany documented as relationType instead of RelationshipMeta Third param of JSONAPISerializer.shouldSerializeHasMany documented as relationType instead of RelationshipMeta Jul 15, 2022
@runspired
Copy link
Contributor

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue 🏷️ doc This PR adds/improves/or fixes documentation
Projects
None yet
Development

No branches or pull requests

2 participants