URGENT: Set on immutable record class returns a records without extended methods

[Methuselah96]

From @bogomazov on Sat, 15 Jun 2019 08:06:26 GMT

What happened

Applying .set() to extended immutable record returns just an immutable record without class methods that were in place in the old Immutable record

How to reproduce

1. Have a class with some getter method extending Immutable record.
2. Create an object using the class.
3. Log the object (notice existing getter method)
4. Log a new object returning from "set" operation.
5. Notice missing class methods.

Copied from original issue: https://github.com/immutable-js/immutable-js/issues/1718

[jdeniau]

@Methuselah96 you can assign that issue to me and set it in the 4.0 milestone. That's a case we have a LOT in our codebase.

[jdeniau]

In fact I read the original issue and it seems to be related to arrow function. I can still investigate on this but it looks like it's not a 4.0 issue 😉

In fact, the arrow function is a property of the Record, that is not defined in the default values, so it seems logical that the arrow function is not kept between immutable instances

[Methuselah96]

@jdeniau Feel free to look into it and create a PR to fix it. I'm not going to assign it to the 4.0 milestone since it's not a regression from 3.0, however if it gets merged in before the rest of the 4.0 PRs, then we'll gladly include it.

[jdeniau]

Possible test to "fix" this if this is fixable :

class ABRecord extends Record({ a: 1, b: 2 }) {
  constructor(val) {
    super(val);

    this.key = "key";
  }

  // non-arrow function
  sum() {
    return this.a + this.b;
  }

  // arrow function
  diff = () => {
    return this.a - this.b;
  }
}

const a = new ABRecord({});
const b = a.set('b', 3);

expect(a.key).toEqual('key')
expect(a.sum()).toBe(3);
expect(a.diff()).toBe(-1);

expect(b.key).toEqual('key')
expect(b.sum()).toBe(4);
expect(b.diff()).toBe(-2);

[jdeniau]

I tried to fix this with stuff like base upon Object.getOwnPropertyDescriptors and Object.defineProperties (see this gist), but I got some weird issue with function, as the functions are stil bounded to old instance.

Thinking about this, I do not really like to fix this, as it is really not in the imutable mood : you mix immutable properties with mutable ones.

The key property in the previous example is a good example.

According to me : either we need to document that this should not be done, or we need to throw an error if a class extending an object has some properties.

What do you think ?