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

Array empty element converts to null #6

Open
OwnageIsMagic opened this issue Feb 23, 2018 · 7 comments
Open

Array empty element converts to null #6

OwnageIsMagic opened this issue Feb 23, 2018 · 7 comments

Comments

@OwnageIsMagic
Copy link

OwnageIsMagic commented Feb 23, 2018

stringify(new Array(3)) /* "[null,null,null]" */
[,,][0] === null /* false */
@ForbesLindesay
Copy link
Member

I would accept a pull request to fix this, but it requires us to manually do the stringifying of Arrays and Objects. That would let us properly handle undefined and Dates even when they are properties of objects/elements of arrays.

@OwnageIsMagic
Copy link
Author

Is it really needed?

@ForbesLindesay
Copy link
Member

I'm confused. This was your issue. If you don't think the problem needs solving, why did you open the issue?

@OwnageIsMagic
Copy link
Author

OwnageIsMagic commented Feb 26, 2018

Just to state that issues exists
I don't know how this package is used
I think that people expects that this package does 1:1 conversion, but this not true for this case
https://github.com/pugjs/js-stringify/network/dependents?dependent_type=REPOSITORY
states that this package used by 17k projects
degrading performance for 17k projs for such edge case is somewhat not a good idea

@OwnageIsMagic
Copy link
Author

OwnageIsMagic commented Feb 26, 2018

btw fix is quite easy

if (Array.isArray(obj))
  return '[' + obj.map(x => stringify(x)).join(',') + ']';

but it will sometimes add +1 to arr length based on trailing comma support of runtime

@ForbesLindesay
Copy link
Member

That example is wrong in a more subtle way:

[,,,,]
// => [empty x 4]
eval('[' + [,,,,].join(',') + ']')
// => [empty x 3]

I don't know whether this is super important to fix or not. I suspect not as sparse arrays are a pretty rare occurrence, but it would be nice to support undefined in arrays and Date objects in deeply nested locations.

@OwnageIsMagic
Copy link
Author

OwnageIsMagic commented Feb 26, 2018

Yes, as mentioned before trailing comma consumes last delimetr
Here is easy fix for that one :)
"engine": { "node": "<6" }
Accesing out of bounds will return undefined, so no problem except array.length
But code like this is already broken by Node (and this change will be semver major, so its ok)

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

No branches or pull requests

2 participants