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(kitsu-core): Deserialize and link nested relations #601

Merged
merged 6 commits into from
Oct 19, 2021

Conversation

mgebeily
Copy link
Contributor

I ran into some infinite recursion during my usage of kitsu-core. Nested relations don't properly deserialize. I fixed it by "caching" previously nested relations in a json object that's kicked around between the methods that need it.

I'd appreciate a review. One of the things I did that I'm not 100% on is having the deepest level object point to whatever the relation was, which causes a circular JSON structure. I can also make that object a leaf node and just early return, which would allow stringification of the resulting object, but it would make different "versions" of the object not contain relationships.

Let me know if this makes sense. Thanks in advance and keep up the great work on an awesome library.

@mgebeily mgebeily changed the title fix(kistu-core): Deserialize and link nested relations fix(kitsu-core): Deserialize and link nested relations Oct 16, 2021
@wopian wopian self-requested a review October 18, 2021 08:40
@wopian wopian self-assigned this Oct 18, 2021
@wopian wopian added this to the 10 milestone Oct 18, 2021
@wopian
Copy link
Owner

wopian commented Oct 18, 2021

I removed the default value that was resulting in an untested branch that was unreachable and used json-stringify-safe to visibly show the output has a circular structure in the tests instead of discarding the circular portion of the output.

I think we can leave it outputting a circular JSON structure without issues though.

…nternally

JSDoc doesn't have anything for this type of thing it seems.
@codeclimate
Copy link

codeclimate bot commented Oct 19, 2021

Code Climate has analyzed commit 713cafd and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

@wopian wopian merged commit 5bb705a into wopian:master Oct 19, 2021
@wopian
Copy link
Owner

wopian commented Oct 19, 2021

Released in 10.0.0-alpha.17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants