-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
hashing breaks for custom objects with "Invalid value used as weak map key" #1643
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
Comments
o_O |
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. |
Is there a workaround for this or is anyone working on a fix? |
Bumped into, what looks like the same, issue. Here is a codepen: https://codepen.io/team/SPT/pen/wOEoXY |
We also encountered this issue and the fix in adamshone@209817e seems to solve. Any chance this can be merged? |
Using this as a workaround 🤦♂️ Date.prototype.hashCode = function () {
return Number(this);
}; |
I've had the same/similar bug manifest in a different way. When calling It took me about 4 hours of debugging to figure out that it was occurring when 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. |
Have this problem also, does anyone know how to fix this when use with Moment library? |
just hit the same issue today |
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 ¯_(ツ)_/¯ |
I hit the same thing after adding couple of |
Was also able to reproduce this using luxon's |
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. |
Thanks! Gonna be a big refactor but hopefully worth it :) |
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. |
Cool will check those out. Thanks :) |
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 If you do try it out be sure to use the |
@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! |
@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. |
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 |
I think the latest changes to
Hash.js
introduce a weird bug when hashing custom objects, more specifically, a class withvalueOf()
overridden. The error being thrown isUncaught 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 aMap
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.
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:
The text was updated successfully, but these errors were encountered: