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

deepEquals always fails on objects with arrays created with seamless-immutable #24

Closed
glantucan opened this issue Jul 28, 2020 · 5 comments

Comments

@glantucan
Copy link

deepEquals always fails on objects with arrays created with seamless-immutable, (or any other library that adds methods to the arrays)
ospec version: 4.1.1

Code:

The following test fails when it shouldn't

let a = Immutable({
    tasks: {
        tasksList: [
            {
                due_date: 1595808000,
                dueMessage: 'Due today',
                daysLeft: 0,
            }
        ],
        computed: false
    }
})
, b = Immutable({
    tasks: {
        tasksList: [
            {
                due_date: 1595808000,
                dueMessage: 'Due today',
                daysLeft: 0,
            }
        ],
        computed: false
    }
})

o('test', () => {
    o(a).deepEquals(b)
})

o.run()

The problem seems to be here: https://github.com/MithrilJS/ospec/blob/master/ospec.js#L543

Ospec is using Object.getOwnPropertyNames on the array so it's finding non-enumerable functions and trying to compare them as well, and so it's finding non-enumerable functions and trying to compare them as well, rather than just ignoring them like it should.

@pygy
Copy link
Member

pygy commented May 17, 2022

@glantucan Not sure how we'll fix this, but in the mean time, you can write your own validator using the .satisfies() assertion.

Schematically:

const  compareImmutable = (reference) => (candidate) => {
  const discrepancies = []
  // actually compare `reference` and `candidate`, populate `discrepancies` if needed
  return {pass: discrepancies.length === 0, message: discrepancies.join("\n")}
}
o("test", () => {
  o(candidate).satisfies(compareImmutable(reference))
})

You'll probably want a recursive helper of the form

_compareImmutable(reference, candidate, discrepancies) {}

@pygy
Copy link
Member

pygy commented May 19, 2022

So, this is not something we can fix in core, as these objects are not deepEquals according to our definition, and out API leaves no room for adding an exception list.

Here's a rudimentary port of deepEquals to support seamless-immutable arrays (a slightly modified version of ospec's deepEquals). Feel free to embellish it to your fit needs.

Edit: better demo

@pygy pygy closed this as completed May 19, 2022
@pygy
Copy link
Member

pygy commented May 19, 2022

Rethinking about this, I may have gotten this wrong... Non-enumerable properties are not compared on objects, we should skip them on arrays too for consistency.

@pygy pygy reopened this May 19, 2022
@pygy pygy closed this as completed in 6379ceb May 19, 2022
@pygy
Copy link
Member

pygy commented May 19, 2022

This is fixed in v4.1.6

@glantucan
Copy link
Author

Thanks for taking care of this :)

@pygy pygy mentioned this issue Jan 20, 2023
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