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.

Sign up for GitHub

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

Jump to bottom

Sparse arrays are treated like arrays of undefined #74

Open
ninevra opened this issue Jul 13, 2021 · 3 comments · May be fixed by #75
Open

Sparse arrays are treated like arrays of undefined #74

ninevra opened this issue Jul 13, 2021 · 3 comments · May be fixed by #75

Comments

@ninevra
Copy link
Contributor

ninevra commented Jul 13, 2021

concordance treats empty slots in arrays like undefined, for both formatting and comparison.

format(new Array(5)) gives

[
  undefined,
  undefined,
  undefined,
  undefined,
  undefined,
]

whereas util.inspect(new Array(5)) gives [ <5 empty items> ]

compare(new Array(5), Array.from({length: 5}) returns true, although these values behave rather differently.
@novemberborn
Copy link
Member

Nice find! If we could detect the gap, we could encode a custom "empty primitive"?

@ninevra
Copy link
Contributor Author

ninevra commented Jul 15, 2021 •
edited

That would make sense to me. Detecting the gap is simple enough, just add if (current in this.value) {} around line 122 in

concordance/lib/complexValues/object.js

Lines 110 to 123 in b30e7c8

createListRecursor () {
if (!this.isList) return recursorUtils.NOOP_RECURSOR
const size = this.value.length
if (size === 0) return recursorUtils.NOOP_RECURSOR
let index = 0
const next = () => {
if (index === size) return null
const current = index
index++
return this.describeItem(current, this.describeAny(this.value[current]))
}

I think any change here would probably(?) be breaking, though, since it would mean introducing a new descriptor tag, meaning old versions couldn't decode new serializations, and existing snapshots of sparse arrays would compare inequal to new snapshots of the same values.

There's also a test case asserting the current behavior at

concordance/test/lodash-isequal-comparison.js

Line 135 in b30e7c8

t.true(isEqual(array, [undefined]))

I reviewed a few other libraries; it looks like chai, jest, and lodash all consider empty array items to equal undefined, while node's assert builtin module does not.

@novemberborn
Copy link
Member

I think any change here would probably(?) be breaking, though, since it would mean introducing a new descriptor tag, meaning old versions couldn't decode new serializations, and existing snapshots of sparse arrays would compare inequal to new snapshots of the same values.

Yea. Let's see if we can get that into AVA 4 — and otherwise we can work out some compatibility between the different snapshot versions.

I reviewed a few other libraries; it looks like chai, jest, and lodash all consider empty array items to equal undefined, while node's assert builtin module does not.

I'm happy to follow assert, sparse arrays do behave differently so it seems useful to reflect that in snapshots and assertions.

@ninevra ninevra linked a pull request Jul 20, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle empty items in arrays and arraylike objects ninevra/concordance
2 participants
@novemberborn @ninevra