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

Throw error on circular references #367

Closed
richarddd opened this issue May 27, 2020 · 9 comments
Closed

Throw error on circular references #367

richarddd opened this issue May 27, 2020 · 9 comments

Comments

@richarddd
Copy link

Currently the library does not throw error when parsing circual references.

Example:

const circular = {
   a : "value"
}
circular.a = circular
JSON.stringify(circular) //throws error
const circular = {
   a : "value"
}
circular.a = circular
qs.stringify(circular) //freezes computer 😣
@ljharb
Copy link
Owner

ljharb commented May 27, 2020

Related to #31; seems like maybe we need the same treatment for stringify.

@Poyoman39
Copy link

Poyoman39 commented Jan 27, 2021

What about a "maxDepth" option ?
I will make a PR for this

@Poyoman39
Copy link

Just to know. What is the targeted browser compatibility of qs ? I would have a solution working with JavaScript "Map" ... but it's not available in IE10 <=

@ljharb
Copy link
Owner

ljharb commented Jan 27, 2021

@Poyoman39 its much older than that, I’m afraid. Map can be shimmed (and should always be), but qs can’t rely on that being the case. However, we could build a fake Map with an array if WeakMap/Map isn’t available

@Poyoman39
Copy link

Ok i did a throw / Map solution (second PR) . I wanted to use Map for performance reasons. But maybe it's a bit of overOptiomisation ? Since querystrings does not contains that much keys ??? It would be possible to do it with a simple array ... les performant, but without requiring a Shim ?

What do you think ?

@Poyoman39
Copy link

Ok last PR, this time with array way :D #395

We have some choice for this one issue :D

@ljharb
Copy link
Owner

ljharb commented Jan 28, 2021

Ok please stop creating extra PRs tho - each PR is a permanent ref on the repo, so how i have 3 PRs to keep in sync :-)

@Poyoman39
Copy link

Ok sry for this ^^ Do you want i close some of them ?

@ljharb
Copy link
Owner

ljharb commented Jan 28, 2021

No, please leave them all open - whatever solution we go with, I’ll update all three PRs to have it as part of landing.

@ljharb ljharb closed this as completed in 63766c2 Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants