Skip to content
This repository has been archived by the owner on Mar 20, 2022. It is now read-only.

Add support for circular references #335

Merged

Conversation

ptshih
Copy link
Contributor

@ptshih ptshih commented May 10, 2018

Problem

Unable to normalize inputs with circular references. I get a maximum call stack size exceeded error.

Solution

This PR is based on #330 with a few changes (based on the fixes recommended there):

  • Used Array.prototype.some for equality check instead of Set to avoid a polyfill
  • Added tests using toMatchSnapshot

TODO

  • Add & update tests
  • Ensure CI is passing (lint, tests, flow)
  • Update relevant documentation

Copy link
Owner

@paularmstrong paularmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would appreciate more information for why you made the choices you did. Also want to see some changes to ensure no performance regressions (JSON.stringify) and not using bitwise operators.

src/schemas/Entity.js Outdated Show resolved Hide resolved
src/schemas/Entity.js Outdated Show resolved Hide resolved
src/schemas/Entity.js Outdated Show resolved Hide resolved
@ptshih
Copy link
Contributor Author

ptshih commented Jun 1, 2018

Thanks for the comments @paularmstrong - I'll tinker around this weekend to try and address the above concerns.

Remove the use of `JSON.stringify` for comparing equality; instead use `Array.prototype.some` to check equality
@ptshih
Copy link
Contributor Author

ptshih commented Jun 3, 2018

@paularmstrong - I made some improvements based on your feedback; it's definitely a much cleaner implementation now.

@ptshih
Copy link
Contributor Author

ptshih commented Jul 1, 2018

@paularmstrong bumping this

@abhishiv
Copy link

Would be great to have this merged!

@ptshih
Copy link
Contributor Author

ptshih commented Oct 24, 2018

@paularmstrong Just curious if there is any reason not to merge this PR?

@arahansen
Copy link

I just hit this issue. Is there any remaining work on this to get this ready to merge?

Copy link
Owner

@paularmstrong paularmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! Looks good.

@paularmstrong paularmstrong merged commit 3e706ec into paularmstrong:master Mar 6, 2019
@lock
Copy link

lock bot commented Sep 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the Outdated label Sep 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants