Map equality issue

[alexw668]

Hi,

First of all, it appears (and it makes sense) that key order in an immutable Map with primitive fields does not matter, as illustrated by the following test:

  it('Key order for Maps with primitive fields does not matter', ()=>{
    const simpleMap =  Map({
      loaded: false,
      working: true
    });    
    expect(simpleMap).to.equal(Map({   // key order different
      working: true,
      loaded: false
    }));
  })

However, if an immutable Map contains one or more key with plain object in it will cause equality to fail if the order of keys is different, as illustrated in the following test (notice I use to.eql, not to.equal):

  it('Map field data order matters!!', ()=>{
    const map =  Map({
      loaded: false,
      working: true,
      message: {first_name: 'john', last_name: 'due'}
    });
    expect(map).to.eql(Map({   // key order the same
      loaded: false,
      working: true,
      message: {first_name: 'john', last_name: 'due'}
    }));
    expect(map).to.eql(Map({   // key order different
      working: true,
      loaded: false,
      message: {first_name: 'john', last_name: 'due'}
    }));
  })

The second expect fails.

Is that an intended behavior or a bug in chai-immutable?

Thanks,
Alex

PS. I am using chai-immutable v. 1.3.0.

[astorije]

This case is actually a duplicate of #23. Your problem is not that key ordering matters with non-primitive parameters, but that you are changing the operator from equal to eql in your examples, the latter being not available in chai-immutable right now.

[alexw668]

I am not sure. Issue #23 is concerned with List. In my case it's about Map. I believe if you have a plain object in a Map, you cannot test equality with .to.equal; you have to use to.eql, which is why the 1st expect in my sample code actually passes (but it would not if I had used to.equal). Theoretically, the last expect should pass but it does not, which I think is a bug. The only difference between the 1st and the last expect is the order of loaded: and working: keys.

The last expect will never pass if to.equal is used.

[astorije]

Oh my, you're right, it is related to the ordering, my bad:

var map = new Map({
  loaded: false,
  message: {
    firstName: 'john',
    lastName: 'due'
  },
  working: true
});

// Passes
expect(map).to.eql(new Map({
  loaded: false
  message: {
    firstName: 'john',
    lastName: 'due'
  },
  working: true,
}));

// Fails
expect(map).to.eql(new Map({
  message: {
    firstName: 'john',
    lastName: 'due'
  },
  working: true,
  loaded: false
}));

(Yes, it's what you have in example, but I am using this in the test file)

I do not define eql, this is chai's default one, which directly uses deep-eql. Could there be a bug there?

[astorije]

Could there be a bug there?

Nop, as this passes:

expect({
  foo: 'bar',
  one: 'two'
}).to.eql({
  one: 'two',
  foo: 'bar'
});

[astorije]

Got it. Here is the output when testing against this example:

      AssertionError: expected { Object (size, _root, ...) } to deeply equal { Object (size, _root, ...) }
      + expected - actual

         "__ownerID": [undefined]
         "_root": {
           "entries": [
             [
      -        "loaded"
      -        false
      -      ]
      -      [
               "message"
               {
                 "firstName": "john"
                 "lastName": "due"
             [
               "working"
               true
             ]
      +      [
      +        "loaded"
      +        false
      +      ]
           ]
           "ownerID": {}
         }
         "size": 3

It means that Immutable stores the key-value pairs in an array, as they are defined. When eql is used, it sees this as a difference.

Anyway, I should probably using Immutable.is or the .equals function that Immutable exposes.

[astorije]

Well, that's a bummer. Chai uses deep-eql, and Immutable structures expose a deep equality function .equals().
The former one does not work with Immutable objects for the reason above.
The latter one does not work with objects:

var map1 = new Map({
  foo: 'bar',
  message: new Map({ bar: 'foo' })
});

// Prints `true`
console.log(map1.equals(new Map({
  foo: 'bar',
  message: new Map({ bar: 'foo' })
})));

var map2 = new Map({
  foo: 'bar',
  message: { bar: 'foo' })
});

// Prints `false`
console.log(map2.equals(new Map({
  foo: 'bar',
  message: { bar: 'foo' })
})));

Right now, your original example will fail because it uses deep-eql by default. If I switch to use Immutable's .equals(), it will fail because you are using an object within your Map...

On one hand, .equals() is having a correct behavior because these 2 sub-objects are essentially different (by the measure of Object.is()). On the other hand, one would probably expect that .eql works either way...

Really, I'm not sure how to fix this. I am open to ideas and suggestions (that would not make chai-immutable entirely redefine deep-eql and Immutable's .equals()...).

[alexw668]

Great explanation. Thanks for your time. I am interested to see if some one has a solution for this.

Alex

[astorije]

Actually, I have two sort-of solutions, very similar in spirit. Nothing ideal though, but might be a start:

• Convert the Iterables to JS objects using toJS() and then use Chai's .eql()
• If we are using .eql() on Iterables, use fromJS on them (assuming fromJS is idempotent, i.e. it will convert plain objects to Maps and arrays to Lists but will leave Iterables as-is) so that sub-elements are converted to Immutable structures, and then use .equals() on them.

I can see many false positives on that (a Map with an array inside will be deemed equal to a Map with a List inside), but I cannot find a better solution.
What do you think of these?

[alexw668]

I think that should work. It would be nice if you (or any adapters) could identify any false positive and come up with a solution to address the situation.

Thanks!

[astorije]

Hey @alexw668, I'll work on something as soon as I get some free time. Depending on how it goes, I'll probably add a caveat to the README if there are false positives that should be expected (if these are only those I listed, it can be easily predicted, and one can add an additional check to balance it; not the cleanest solution, but better than nothing, right?).

In the meantime, that would be very helpful if you can list different cases you can think of as examples. By covering different sorts of inputs, I'll be able to feed the tests and make sure we do not ship any regressions if we can improve the selected solution over time.
Now that I can think of it, it will be also helpful to add tests for the known false positives and mark the tests as ignored. That way we can keep track of them and easily add them to the list if we improve the solution.

Thanks!

[alexw668]

Hi @astorije. Sorry for late response. Looking back at what I did in my immutable Map or List, I realized that it's not a good practice to mix mutable data (like plain object) within an immutable object, particularly based on the principal of Flux (or React Redux). So I made sure mutable data is mixed in my immutable object. By doing so, I think chai-immutable does a very good job. I don't ever use to.eql to test for equality. I always use to.equal for that. In that case, the order of fields in Map or in List won't matter.

Because of this, I don't think you want or should to deal with various cases I mentioned or other developers may have. I think in your README, you could state that it's not a good idea to have mutable data contained in immutable object, and if a developer does not follow good practice, he/she would need to test things differently.

What do you think about my statement here? Sorry for bringing this "issue" up; I should have followed the good practice at the first place.

Thanks very much!

[astorije]

So you think having arrays and objects in Lists and Maps is not a good practice? Where did you take that statement from, by curiosity? I don't know the principle of Flux, by the way.

Your recommendation is to leave the code as is, right?
No worries for opening this issue, the discussion that came from it was very interesting.
I will update the README, and will appreciate you give your comments on my edits to let me know if it's suitable or not according to you. You sharing how you came up with this good practice and/or where you found it will be helpful to update the README.

Thanks!

[alexw668]

Yes, I read it on some one's blog. Also here is a review about the Flux pattern (Redux is an implementation of that pattern): https://facebook.github.io/flux/docs/overview.html.

I believe you've heard of React.js, a Javascript library for building UI. It achieves lots of its goal with a virtual DOM; having a pure immutable state, it can quickly calculate the diffs between the virtual and actual DOM and renders things quickly. Having mutable objects in immutable Map and List will confuse the algorithm, as I've seen in our project. In other words, having mutable data in immutable object will turn that object mutable.

[astorije]

Thanks for all these precisions, @alexw668.

I have opened #25 with additions to the README and tests to to make sure our assumptions were true.
Before merging, I'll wait for your comments regarding both the README edits (I'm not native and I didn't stumble upon this problem myself so feel free to tell me if I am not being clear), and the tests in case you can think of cases we might want to tests that I forgot.
After that, I'll also make a patch release. Although this doesn't touch the library's code, I want to update the npm package page with the precisions when we can.

Thanks for your help on this, @alexw668!

[alexw668]

@astorije I like what you changed in README. If I have any more suggestion, I would also add a reference to the case for Immutability which emphasizes that immutable data structures should be treated as values rather than objects.

Thanks for your time and work!

[astorije]

I'd rather leave that out for the following reasons:

• I'd expect a reasonable developer to have read Immutable's README in order to use Immutable and chai-immutable
• This paragraph doesn't explicitly says you should not be mixing mutable and immutable objects (it is a broader, yet very valid discussion/context, but not specifically directed at our current issue)
• I'd prefer keep the README short if possible
• Well, the README links to this current issue which says one should read that paragraph anyway :-)

Let me know if this seems reasonable and please give your 👍 to #25 or add any more comments.

[astorije]

@alexw668, ping?

[astorije]

I'll take your silence as a 👍 and will merge #25 then. I will release a minor version right after this and will also close this issue.

Thanks for reporting this in the first place!

[astorije]

Released in version 1.4.0, closing this.

[alexw668]

Thanks for your work! Sorry for not responding sooner because 1) I have been too busy at work, and 2) I thought I already endorsed your change. ;-)

[astorije]

No worries, all good! And plenty to do on the latest issues now :-)

[kmckee]

This probably isn't the best place for this, but, it seems kind of related. I'm new to working with immutables so go easy on me.

I noticed that in certain scenarios, like the example below, I get output from the tests that makes it really difficult to understand what's wrong. Am I using the library incorrectly or is this something that could be enhanced?

        it('does not give helpful test output?', () => {
            const expected = Map({
                entries: List.of('Trainspotting')
            });
            const actual = Map({
                entries: List.of('28 Days Later')
            });
            expect(expected).to.eql(actual);
        });

That test gives me the following output:

AssertionError: expected { Object (size, _root, ...) } to equal { Object (size, _root, ...) }

If I convert my immutables to regular JS objects, like below, I get output like what I was expecting:

        it('has helpful output with JS objects', () => {
            const expected = Map({
                entries: List.of('Trainspotting')
            });
            const actual = Map({
                entries: List.of('28 Days Later')
            });
            expect(expected.toJS()).to.eql(actual.toJS());
        });

Output:

AssertionError: expected { entries: [ 'Trainspotting' ] } to deeply equal { entries: [ '28 Days Later' ] }
      + expected - actual

       {
         "entries": [
      -    "Trainspotting"
      +    "28 Days Later"
         ]
       }

Again, sorry to crash this thread :)

[astorije]

@kmckee, actually, this is unrelated. I do believe though that it is related to #7. Can you confirm? If it is, please post there, and if it isn't, please create a new issue for this. I'm about to look into this.

[black-snow]

Anything new on this?

[astorije]

@black-snow, about what specifically? Lots was said on this thread 😄

[renanvalentin]

@astorije I've just got in a weird situation:

var z = Record({a: []});
var a = new z({a: [1]});
var b = new z({a: [1]});
expect(Immutable.is(a, b)).to.be.true; // false

Now I don't know if I can use Array inside of a Record. Also I was thinking about using fromJS inside of the chai-immutable. But I've my concerns about that.

[renanvalentin]

So I've changed the code and it's working. I've switched from [] to Immutable.List.

var z = Record({a: List()});
var a = new z({a: List([1])});
var b = new z({a: List([1])});
expect(Immutable.is(a, b)).to.be.true; // true

[MicahZoltu]

The inability to do deep comparisons of immutable objects that have data structures not from ImmutableJS along the way is problematic. I'm using TypeScript with readonly properties (language level immutability).

class Foo {
	readonly a: string;
	readonly b: number;
	readonly c: List<string>;
	constructor(a: string, b: number, c: List<string>) {
		this.a = a;
		this.b = b;
		this.c = c;
	}
}

// this is a compiler error, because Foo is immutable
// new Foo("a", 1, List<string>()).a = 5;

const first = List<Foo>(new Foo("hello", 5, List<string>()));
const second = List<Foo>(new Foo("hello", 5, List<string>()));
expect(first).to.deep.equal(second); // fails

So in this case, the code is sound as I am only using Immutable datastructures all the way down. However, I'm not using datastructures provided by the ImmutableJs library all the way down.

Unfortunately toJS doesn't help here because I have a nested Immutable datastructure and I can only apply toJS to the top level, I can't do it recursively all the way down the object graph (as far as I know).