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

fix(relationships): fix one relationship to many empty #186

Merged
merged 1 commit into from May 4, 2020
Merged

fix(relationships): fix one relationship to many empty #186

merged 1 commit into from May 4, 2020

Conversation

SancheZz
Copy link

info
docs
to-one relationship should return data = null
to-many relationship should return data = []

subject
At the moment, we get an error when destructuring an empty array on line 328.

@CodingItWrong
Copy link
Contributor

@SancheZz Thanks for this fix! Could you add a test covering it? Here's a similar test passing in a present relationship; you could add a test passing in an empty relationship.

@SancheZz
Copy link
Author

@CodingItWrong Thank you! :) I added a test for that fix. I have changed the update record behavior. Old relationships will be removed first and then new relationships will be added from the record.

@CodingItWrong
Copy link
Contributor

@SancheZz Thanks for adding this test!

I'd like to request one enhancement. On like 327 you're using the relationship name as the name of the resource, which works for your test and most basic cases, but is not always the case. For example, you might have a relationship named authors, but the resource name is people.

Instead, it's safer to look up the name of the resource from one of the records in the array. You can see me doing this here: 3ed8ea4#diff-602c4b25d8a2c1cae4aefd3464d69f51R354

If you add the following above your dispatch call to get the type, and use that to dispatch to instead, the test will still pass, and it'll handle more cases

const { data } = oldRecord.relationships[name];
let type;
if (Array.isArray(data)) {
  ({ type } = data[0]);
} else {
  ({ type } = data);
}

Another note: you first clear the old relationships, then store the new record, then store the new relationships. I think in the future I might work on changing the code so that we first store the record, then store the new relationships or clear out a relationship that was empty. I experimented with doing this and it was tricky, so I see why you didn't do it that way. But if you know of any reasons for me not to do it that way in the future, please let me know!

Thanks again for contributing. After you add the type lookup above, I can merge and do a release to include this fix.

@SancheZz
Copy link
Author

SancheZz commented May 3, 2020

@CodingItWrong Thank you very much! You are right! I need to use relationship and type of each resource which will be removed. I have added helper function getRelationshipType to get a type of a resource.
I think a new major or minor version should contain a graph of resources. :) Each operation with the JSONAPI store should update the graph. :)

@CodingItWrong
Copy link
Contributor

@SancheZz Nice helper function, I think I'll be able to use that elsewhere.

Good big-picture thoughts too; I'll think it over.

Thanks again for contributing! I'll release this shortly.

@CodingItWrong CodingItWrong merged commit d2adb11 into reststate:master May 4, 2020
@SancheZz SancheZz deleted the empty-relationships branch May 4, 2020 19:38
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.

None yet

2 participants