Records compare equal to Maps

[glenjamin]

See original discussion here: https://github.com/glenjamin/transit-immutable-js/pull/13#issuecomment-172021813

Notable points:

var FooRecord = Immutable.Record({ a: 1, b: 2 }, 'foo');

// This assertion passes
expect(new FooRecord()).to.eql(new Immutable.Map({a: 1, b: 2}));

// Because this is true
Immutable.is(new FooRecord(), new Immutable.Map({a: 1, b: 2}));

And this is currently by design: https://twitter.com/glenathan/status/688050710047539200

I think it'd be useful to provide a way to opt into a stricter comparison in chai-immutable. I'm unsure whether or not I think it should be the default at this point.

One suggested approach is to say that immutable object should be both equal, and stringly equal - as records include their names in the string output. This doesn't handle records with the same name though - so perhaps it would need something more complicated.

/cc @corbt

[astorije]

Well, that sucks :-) I must admit, I have never played with Records and prior to a minute ago, I had never even read its doc!

I agree with you, .to.equal and .to.eql should fail if the types are different. Unless we are talking duck-typing and the methods accessible for the Record are the same than for the Map. Is that the case? If it is, then there is no reason I think to make this fail. What do you think?

If it is not, this needs fixing.

I'm afraid the stringly equal way might not be very robust: I have seen times when the string output of 2 Maps were showing keys in different order. A maybe poor alternative could be to do something along the lines of:

// Either both are Maps or none of them are
Immutable.is(a, b) && Immutable.Map.isMap(a) === Immutable.Map.isMap(b)

If one is a Record and the other is a Map, this would fail. Of course, that's assuming Immutable.Map.isMap(new FooRecord()) returns false...

What do you think?

[glenjamin]

That wouldn't work deeply - it might be best to fix this upstream in Immutable, Lee has since said he'd be ok with that.

[astorije]

Right, it wouldn't...

Sure, that'd be better if this can be fixed upstream. In the meanwhile, a temp quickfix or a note in the README might help further users of chai-immutable. Do you want to take a look at this? :)

Also, I'm not sure about Records: in the case of your FooRecord, does it share the same methods and properties than a regular Map?

[glenjamin]

They're similar, but I'm unsure if they have all the methods.

I also don't really use records, this only came up because of a PR into my transit lib - @corbt is probably best placed to advise.

[corbt]

Records do implement most of the same methods as maps (although I'm not sure about all of them), plus the ability to access the defined fields using property access syntax (recordInstance.myProperty). Since they also implement the get equivalents (recordInstance.get('myProperty')) they can usually be a drop-in replacement for a map as long as you're only getting/setting the defined record fields.

However, semantically they're very different. In a codebase that uses both records and maps, I would expect a record to be used for anything where the keys are defined and finite, and a map to be used anywhere that the keys are unknown/dynamic. A Record has similar semantics to a struct in C or a data-only class in Ruby/Java, whereas a Map is more like a Map/HashMap in any of those languages.

Obviously an immutablejs map can be used for both use cases, but in a codebase that includes both records and maps I would expect the distinction to hold. So this is a long way of saying that if I'm expecting a Record and get a Map, that should be an error, because as long as I'm using both there's probably a good reason and an expectation that they're two different things. :smile:

[renanvalentin]

Hey guys, what is the expected behavior in this case?

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

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

Thanks!

[glenjamin]

@renanvalentin This isn't really related to this repo, as you're using Immutable.is directly - so its all code in immutable-js itself.

The behaviour you're seeing is correct though - normal javascript arrays are compared by identity - not by value. If you want to compare an indexed collection by value you'll need to use an immutable List instead of an array.

[renanvalentin]

Hi @glenjamin, Thanks for your explanation!

I'm using chai-immutable in this and it was failing:

var z = Record({a: []});
var a = new z({a: [1]});
var b = new z({a: [1]});
expect(a).to.equal(b); // false

But I've solved it changing the array to a Immutable.List

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

[astorije]

Hey @glenjamin, @corbt, and @renanvalentin! I know it's been an awful while, but trying my luck still: does any of you want to further help with this? 🙏