expect(() => Record(fooInstance)).toThrowErrorMatchingSnapshot()). The problem with writing the error two places is that if it should ever get out of sync it's hard to tell whether the source of truth is the test code or the app code. Also if anyone rewrites a bunch of errors all at once the copy and pasting is a real pain vs just running npx jest -u

If we know it's a record then there's no reason we can't take the correct action right? The error is caused in this case by the use of Object.keys(). The original bug suggests calling toJS, which almost makes sense except that it's a deep transformation and could potentially mangle information about what is stored in the record. I'd say you should call defaultValues.toObject() here. That is implemented at the collection level, so if you just check using isCollection I think you'd solve a whole family of related issues. Of course some kinds of collections may have very weird results.

I think the best of all worlds would be this: check isCollection(defaultValues) then if !isKeyedCollection(defaultValues) throw an error.

@conartist6 The fact is that isCollection and isKeyed returns false as the implementation does check the existence of @@__IMMUTABLE_ITERABLE__@@ and @@__IMMUTABLE_KEYED__@@, which are undefined for Record instances.

I changed the code with toObject as you suggested to simply fix the original issue, as it seems to dangerous to check the default value in the Record method (for now at least as it may induce bugs for weird objects).

The fact is, that now you can not pass a Map as defaultValues (but it wasn't possible either).

As a matter of fact, the record defaultValues can be really weird, as it accepts everything where you can call Object.keys on (even if the typescript types only allow instances of Object).

Maybe I should still add an error like this

if (typeof defaultValues !== 'object' || defaultValues === null) {
  throw new Error(
    'Can not call `Record` with a non-object as default values. You may want to use a javascript object.'
  );
}

and then later check if it's has the toObject or toJS method as default values ? 🤔

What do you think ?

My point generally is this: we should prefer working over throwing an error when both are possible. I forgot that records are not collections, but it doesn't change my general thought. In your case of course you didn't mean to pass a record, but think about somebody who did mean to do that. If we can understand what they meant, why should we not just do it?

I am fine with throwing on collections. But for the same reasons you took this up initially it would be nice to tell the user what they did wrong. Of course they wouldn't be in an infinite loop, but the results would definitely be surprising.

I do not know what is the direction of immutable about this point. The two options are valid: either throw to avoid unexpected comportment or try to correct user code whenever possible. 🤷‍♂️

Apparently immutable 3.8 does not address neither one or another solution on this specific feature.

I finally did two things:

• accept Record as a constructor
• throw when the default values are not an object

This way, the 3.8 comportment still works as expected, and there is an explicit error message when we try to inject weird default values.

We could eventually manage some other selected types (like Map, which is really similar to a JS object)

Okay, I did a little bit more digging to see what the other Immutable data structures do. Many of them end up calling keyedSeqFromValue, indexedSeqFromValue, or seqFromValue (code) which throws a TypeError if they're not of a proper type. Based on that I'm fine with throwing an error on immutable instances and non-objects.

I would still prefer not implicitly converting the Record since I don't think it's clear what the author's intention is.

Thanks for sticking through these long conversations!

Done

I'm fine this, but I do disagree with @Methuselah96. Yes this one person passed a record and it wasn't what he meant. But the purpose of records is to behave like objects. There happens to be this one flaw in their facade as regards Object.keys, but I don't think we want to save that guy from himself. If the program can be executed we can't fix programmer error at runtime... Typescript probably could have caught the problem though.

I'm going to further defend my position, but feel free to stop me when this has become counter-productive.

I can't think of a case where I would want to use a Record as the defaultValues for a new Record in any code I've written, and I would rather get an error (and probably uncover a bug) and be forced to call .toObject() on the Record to make my intention clear (if it wasn't a bug). I would prefer to be saved from myself in this scenario.

I do agree that records are to behave like objects, but they're more heavy-weight because you have to define the structure of the "object" before you can actually create the "object." That's why I have a hard time believing that someone would choose to use this more heavy-weight "object" in order to just use as default values for another heavy-weight "object."

And, yes, hopefully TypeScript could have caught the problem, but I'm thinking it would require some special type guards against Records?

Yeah I agree with you that a legitimate use is pretty unlikely. I'm giving my approval to what @jdeniau has got now.