Immer: Problem with recursive use

[kachkaev]

I'm trying to call produce recursively to process a data tree, but am getting this exception in some cases:

TypeError: Cannot perform 'get' on a proxy that has been revoked

My guess is that immer unintentionally leaves some of the proxies behind and this is why the exception is triggered later in the code.

Here's the original (broken) code, the goal of which is to replace all occurrences of Date or moment leafs into strings:

import produce from "immer";
import { isArray, isDate, isPlainObject, keys } from "lodash";
import { isMoment, Moment, utc } from "moment";

const formatMoment = (m: Moment) => {
  const [hours, minutes, seconds] = [m.hours(), m.minutes(), m.seconds()];
  if (hours + minutes + seconds) {
    if (seconds) {
      return m.format("YYYY-MM-DD HH:mm:ss");
    }
    return m.format("YYYY-MM-DD HH:mm");
  }
  return m.format("YYYY-MM-DD");
};

const transform = (doc) => {
  if (isDate(doc)) {
    return formatMoment(utc(doc));
  } else if (isMoment(doc)) {
    return formatMoment(doc);
  } else if (isArray(doc)) {
    return produce(doc, (draft) =>
      draft.forEach((el, i) => (draft[i] = transform(el))),
    );
  } else if (isPlainObject(doc)) {
    return produce(doc, (draft) => {
      keys(draft).forEach((key) => {
        draft[key] = transform(draft[key]);
      });
    });
  }
  return doc;
};

export default transform;

Expected I/O example:

input: {
  x: moment.utc("2017-01-02"),
  arr: [
    "hello",
    42,
    new Date(2017, 0, 2, 10, 0),
    {
      time: moment.utc("2017-01-02 10:00:01"),
    },
  ],
}
output: {
  x: "2017-01-02",
  arr: [
    "hello",
    42,
    "2017-01-02 10:00",
    {
      time: "2017-01-02 10:00:01",
    },
  ],
}

And here's a slightly clumsier version of the same module, but with no sub-calls of produce. It works:

import produce from "immer";
import { get, isArray, isDate, isPlainObject, keys } from "lodash";
import { isMoment, Moment, utc } from "moment";

const formatMoment = (m: Moment) => {
  const [hours, minutes, seconds] = [m.hours(), m.minutes(), m.seconds()];
  if (hours + minutes + seconds) {
    if (seconds) {
      return m.format("YYYY-MM-DD HH:mm:ss");
    }
    return m.format("YYYY-MM-DD HH:mm");
  }
  return m.format("YYYY-MM-DD");
};

const transform = (doc) => {
  if (isDate(doc)) {
    return formatMoment(utc(doc));
  } else if (isMoment(doc)) {
    return formatMoment(doc);
  } else if (isArray(doc)) {
    return produce(doc, (draft) =>
      draft.forEach((el, i) => (draft[i] = visitDraft(draft, [i]))),
    );
  } else if (isPlainObject(doc)) {
    return produce(doc, (draft) => {
      keys(draft).forEach((key) => (draft[key] = visitDraft(draft, [key])));
    });
  }
  return doc;
};

const visitDraft = (draft, path) => {
  const node = get(draft, path);
  if (isDate(node)) {
    return formatMoment(utc(node));
  } else if (isMoment(node)) {
    return formatMoment(node);
  } else if (isArray(node)) {
    node.forEach((el, i) => (node[i] = visitDraft(draft, [...path, i])));
  } else if (isPlainObject(node)) {
    keys(node).forEach(
      (key) => (node[key] = visitDraft(draft, [...path, key])),
    );
  }
  return node;
};

export default transform;

The broken version uses immer's produce recursively, which means that when a node in a tree is visited, the function's argument is an unfinalized Proxy, not a plain object. Although there exists an internal function called finalize, I could not find how to import it. Can it be a solution for recursive calls?

import produce, { finalize } from "immer";

// const formatMoment = (m: Moment) => ...

const transform = (doc) => { // <--- doc is never a Proxy now
  if (isDate(doc)) {
    return formatMoment(utc(doc));
  } else if (isMoment(doc)) {
    return formatMoment(doc);
  } else if (isArray(doc)) {
    return produce(doc, (draft) =>
      draft.forEach((el, i) => (draft[i] = transform(finalize(el)))), // <--------------
    );
  } else if (isPlainObject(doc)) {
    return produce(doc, (draft) => {
      keys(draft).forEach((key) => (draft[key] = transform(finalize(draft[key])))); // <---
    });
  }
  return doc;
};

__PS__: My environment is node 9.5.0 (+TypeScript).

[mweststrate]

I'm not sure this problem is solvable without introducing confusion somewhere else. But if you could create a PR with some unit tests for this problem, that would be a start. We could for example opt to automatically finalize proxies that are passed as state to a producer. (after all, that instance shouldn't be edited anymore _after_ passing it to a producer since you already produce a new state for it..)

[mweststrate]

The issue might have been fixed by 1.1.3. Since this issue lacks a reproduction, now way to tell. Closing for now. Feel free to re-open with a reproduction if the issue isn't solved.

[kachkaev]

Thank you @mweststrate! I'll report back once I have a look.

[kachkaev]

Unfortunately, the bug appears to be still there, even after installing immer 1.1.3. I'll try to submit a reproduction, but this is unlikely to happen before a weekend that is 10 days from now. If anyone is fancy to build up a unit test out of my example above, feel free! The first code block should be sufficient.

[kachkaev]

@mweststrate I was able to reproduce the bug in https://github.com/mweststrate/immer/issues/118, feel free to have a look!

Could you please reopen this issue? I don't have enough permissions to do so :–)

[mweststrate]

Thanks for the clear test PR @kachkaev!

I investigated a bit and the problem it exposes is that there are no clear semantics when passing a _draft_ to another producers. For example:

const textUpperCaser = produce(draft => {
   draft.text = draft.text.toUpperCase()
})

const processor = produce(draft => {
  draft.forEach((item, i) => {
     draft[i] = textUpperCaser(item)
     // Line X:
     draft[i].count = 0 // Should this throw? or is it ok?
  })
})

processor([{
   text: "test",
   count: 1
}])

The question is, what is the nature draft[i] after line X?

1. according to textUpperCase draft[i] is now frozen / finalized state, and wont be modified anymore. After all, it produced immutable state
2. but according to processor, we are in the middle of a producer function, so we should be able to edit any state in the tree freely

There are few solutions to this:

• 1. Simply forbid passing proxies to a next producer. That would solve this problem/ force that the process is wrapped in produce only once. Downside: this will kill the typical Redux pattern to call reducers from reducers if they are all producers.
• 1. finalize the draft before passing it to another producer. (and return a finalized object after that producer). That would make line X throw. However, this has a few problems as well: First it is extensive / hard to finalize objects, recursively, in the middle of a producer (especially in the ES5 version). Secondly, any dangling references will now become revoked proxies:

const child = item.child
textUpperCase(item)
console.log(child.name) // throws! child is a revoked proxy because `item` was already finalized

• 1. make recursive produce calls no-ops, this means that a producer that receives a proxy just runs, but doesn't keep its own proxy adminstration or finalizes anything. This would make the above all work (including the modification after line X), and is the most efficient. But still it feels a bit like a contract violation of the textUpperCase producer; neither the state it received, or the state is returned is finalized or mutable when called form another producer. And any modification to its basestate will actually really modify its basestate, because baseState === draft. This is implemented atm in #126
• 1. make a deep clone of the current state of the draft when passing it to a sub producer. This sounds semantically the most sound, but very expensive as well :)
• 1. Proxy proxies. This is might be the proper solution, (but might still throw at line X with auto freeze enabled!), probably complicated in the ES5 version as well. it would mean that isProxy checks would become scoped per producer, and objects be proxied multiple times

To be continued :)

[mweststrate]

Released as 1.3.0

[kachkaev]

Thank you @mweststrate! I upgraded to [email protected] and can confirm that I no longer experience the bug 🎉

[aleclarson]

@mweststrate You never stated which option you went with. Looks like you went with option 3, which has a notable side effect. Developers should _never_ use try..catch within a producer if they expect to rollback a nested producer by throwing. Doing that will not actually rollback any changes since the nested producer is using its parent's draft.

I think the correct solution is to reuse the draft state's base and copy, but swap out the rest until the nested producer returns. If a nested producer throws, we simply pop the draft state, without losing the parent's draft state (unless the parent never catches the error thrown by the nested producer, of course). When the nested producer returns, we merge its draft state into the parent's draft state.

[mqklin]

I have Cannot perform 'get' on a proxy that has been revoked as well, but it's very hard to describe the problem

[mweststrate]

@mqklin please open new issues instead of commenting on old ones, and include details otherwise nobody will be able to help. But the premise itself is pretty simple, draft objects cannot be used once the producer function has ended. So if you get this error, you either are callign an async process from a producer, or you are storing references to the draft somewhere, which you shouldn't

[mqklin]

@mweststrate now I understand, thanks for the clarification. I'm not sure if the error is produced by immer, so can't open new issues now.