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: JSON.stringify cannot serialize cyclic structures. #205

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aomkoyo
Copy link

@aomkoyo aomkoyo commented Nov 5, 2024

  • fix JSON.stringify cannot serialize cyclic structures. with function removeCircularReferences

image

@aomkoyo aomkoyo requested a review from Deivu as a code owner November 5, 2024 07:07
@0t4u
Copy link
Member

0t4u commented Nov 5, 2024

where is this error originating from? specifically which function call and payload is causing this issue?

@aomkoyo
Copy link
Author

aomkoyo commented Nov 5, 2024

from payload after adding and send playing song, There will be corrections in every relevant situation.

@Deivu
Copy link
Member

Deivu commented Nov 6, 2024

This do look like a bug, but I dont think going the route of removing the circular reference is also the best solution, cc: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value

I propose that we should take account circular references and use / implement something that can serialize those

Edit: though I'm open for suggestions, do we want to take those into account, or should we just remove it

@Deivu Deivu added the bug Something isn't working label Nov 6, 2024
@aomkoyo
Copy link
Author

aomkoyo commented Nov 6, 2024

I'm not sure, it depends on the way. Is there anything else that can be fixed?

@0t4u
Copy link
Member

0t4u commented Nov 6, 2024

I don't believe shoukaku without modifications is able to have circular references in data being sent, can you send a sample payload? Are you using a modification/extension of shoukaku (like kazagumo)?

@aomkoyo
Copy link
Author

aomkoyo commented Nov 6, 2024

I have changed my payload. But if you use this just in case, there won't be any problems in the future. you can close this

@Deivu Deivu added the blocked Something is preventing this pull from being merged/issue from being solved label Nov 10, 2024
@Deivu
Copy link
Member

Deivu commented Nov 10, 2024

For the meantime I'm blocking this, serializing cyclic references can cause some issues, like possibly a very big payload to serialize, that can cause event loop hangs

@0t4u
Copy link
Member

0t4u commented Nov 10, 2024

I would suggest to catch this specific error and rethrow it with a reference to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something is preventing this pull from being merged/issue from being solved bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants