hashing breaks for custom objects with "Invalid value used as weak map key"

[UnsungHero97]

I think the latest changes to Hash.js introduce a weird bug when hashing custom objects, more specifically, a class with valueOf() overridden. The error being thrown is Uncaught TypeError: Invalid value used as weak map key.

It's strange because it doesn't happen until a certain point, a very specific point. It happens when invoking .set() on a Map on the 9th attempt. For the first 8 attempts, all is good. On the 9th attempt, error.

To reproduce, I ran this bit of code in the console at https://facebook.github.io/immutable-js.

class Example {
  constructor(id, key) { this.id = id; this.key = key; }
  valueOf() { return `${this.id}.${this.key}`; } 
  toString() { return `${this.id}.${this.key}`; }
}

const m = Immutable.Map().asMutable();
m.set(new Example('test', (new Date()).toISOString()), Math.random()); // 1, ok
...
m.set(new Example('test', (new Date()).toISOString()), Math.random()); // 8, ok
m.set(new Example('test', (new Date()).toISOString()), Math.random()); // 9, ERROR

This has worked for me for quite a while. It breaks with release v4.0.0-rc.11.

Here's a screenshot of the console:

[conartist6]

o_O

[joakim-hagglund]

The magic number 9 is due to the MAX_ARRAY_MAP_SIZE check in ArrayMapNode.update where it uses a simple array for small numbers of keys, and only calls createNodes for larger numbers, so that is normal.

The bug itself seems to be in Hash.js from line 34:

if (o.valueOf !== defaultValueOf && typeof o.valueOf === 'function') {
    o = o.valueOf(o);
}
return hashJSObj(o);```

`hashJSObj(o)` here gets a string, but expects an object.
In the previous code the `o = valueOf(o)` assignment were performed first thing, and then treated as if it was the original input paramater. 

[jamiedavenport]

Is there a workaround for this or is anyone working on a fix?

[tyger]

Bumped into, what looks like the same, issue. Here is a codepen: https://codepen.io/team/SPT/pen/wOEoXY
Check console of the browser.

[kwek]

We also encountered this issue and the fix in adamshone@209817e seems to solve. Any chance this can be merged?

[futpib]

Using this as a workaround 🤦‍♂️

Date.prototype.hashCode = function () { 
	return Number(this);
};

[sureshjoshi]

I've had the same/similar bug manifest in a different way.

When calling toSet() on a Map<string, T>, in some conditions (that, for the life of me I couldn't figure out a small reproduction case for), I ran into the following error: TypeError: Attempted to set a non-object key in a WeakMap

It took me about 4 hours of debugging to figure out that it was occurring when hashCode() was being called on some of my Records. For some reason, calling toSet(), toSetSeq() (and maybe one or two more) threw exceptions... toList(), toMap(), toArray() all worked fine.

In any case, I merged @adamshone's PR - and it fixed my issue too. Just wanted to post this to save someone some grief when Googling down the road.

[abrcdf1023]

Have this problem also, does anyone know how to fix this when use with Moment library?

[lukas1994]

just hit the same issue today
when is this PR getting merged?

[adamshone]

I fixed it over a year ago but I think this project has been abandoned. We ran with a fork for a while and eventually switched to https://github.com/immerjs/immer

¯_(ツ)_/¯

[mattleonowicz]

I hit the same thing after adding couple of Dates to Set

[Zaidos]

I hit the same thing after adding couple of Dates to Set

Was also able to reproduce this using luxon's DateTime types. Haven't been able to find a work around yet.

[lukas1994]

This is happening more frequently. Has anyone figured out a solution? Is immutable-js actually not maintained anymore?

[JoshuaToenyes]

This is happening more frequently. Has anyone figured out a solution? Is immutable-js actually not maintained anymore?

@lukas1994 there hasn't been a release since Oct 30, 2018, which is unfortunate. We're using immer now, which is fantastic and actively developed and maintained.

[lukas1994]

Thanks! Gonna be a big refactor but hopefully worth it :)
Do you have any advice for someone moving from immutable-js to immer? Is there anything I should be careful with?

[JoshuaToenyes]

Gosh, that's definitely worth a blog post!

Honestly, I've found immer a lot easier to work with, especially with Typescript (typing with Immutable, specifically Records got really tricking in some cases). I don't have a list of specific things to watch out for off the top of my head. But if you're interested in reading up on a larger project that refactored from Immutable to immer, checkout out Slate: ianstormtaylor/slate#2190, ianstormtaylor/slate#2345, and their milestone for tracking the process https://github.com/ianstormtaylor/slate/milestone/3.

[lukas1994]

Cool will check those out. Thanks :)

[conartist6]

I'm not as familiar with Immer, but it doesn't look like it has the same functional APIs. If you need functional APIs I recommend iter-tools (which I maintain). It works with Maps and Sets (and any iterables), and has many of the methods you'd be familiar with from the Immutable.Seq type.

If you do try it out be sure to use the @next version. 7.0.0-rc.0 is more stable than 6.x.

[JoshuaToenyes]

@conartist6 @lukas1994 - yes, immer has a dramatically different API. It's focused only on immutability, basically, whereas Immutable is a full-featured data structure library.

Thanks for mentioning iter-tools. I haven't seen your lib before, but I'll check it out!

[sureshjoshi]

@lukas1994 For what it's worth, I also switched to Immer from Immutable. It was mildly tedious, but pretty easy and only took an hour or two for a mid-sized project. I'm using Typescript, so it was all compilation errors, zero runtime errors.

Unfortunately, Immutable (which is pretty awesome) ends up being a very leaky abstraction, so it was littered everywhere. Had I thought about this at the time, I could have hidden it away a bit more, but removing all those calls littered everywhere was the bulk of my time.

I decided not to make that same mistake with Immer. At lot of immer examples also show something like redux, which I don't use. My apps are typically a Clean Arch style, so they have underlying domain entities - which are the things most important things that I want to be immutable. So, I created a BaseEntity class which is the only file that contains references to ImmerJS, and it itself is only a constructor and two methods.

I haven't touched this code in a while, since it's worked - so there might be better ways to do this with the latest immer.

export abstract class BaseEntity implements Entity {
    public readonly id: string;

    constructor(props: Partial<ConstructorType<BaseEntity>> = {}) {
        const { id = "" } = props;
        this.id = id;
        (this as any)[immerable] = true;
    }

    public set<K extends keyof this>(key: K, value: this[K]): this {
        const result = produce(this, (draft) => {
            draft[key] = value;
        });
        return result;
    }

    public copyOf(obj: Partial<this>): this {
        const result = produce(this, (draft) => {
            for (const [key, value] of Object.entries(obj)) {
                draft[key] = value ?? this[key];
            }
        });
        return result;
    }

And it gets used like:

const foo = new Foo({
    value1,
    value2,
    value3,
    value4,
    value5,
    value6,
});

const newFoo = foo.set("myKey1", newValue1);

const newerFoo = foo
    .set("myKey1", newValue1)
    .set("myKey2", newValue2)
    .set("myKey3", newValue3);

const newestFoo = foo.copyOf({
    myKey1: newValue1,
    myKey2: newValue2,
    myKey3: newValue3,
});

Not perfect, but has worked for me for a year.

[conartist6]

I've incorporated this fix into immutable-js-oss#17. We hope to be merging the community maintained fork back into immutable before we release. Otherwise we'll release immutable-oss@4.0.0.

[jdeniau]

Thank you for your bug report. The immutable-js oss fork will soon be merged and a fix for this issue has been included in #1833. Once this PR is merged, this issue will be resolved in the main branch. We will then do our best to to release the 4.0.0 version.

Commit reference: 45196e3