Immer 11 breaking optimistic updates

[JacobJaffe]

Hi, I've encountered a very weird bug.

I have reproduced this behavior on rtk 2.9 and 2.11. When locking immer to 10.0.3, the bug is fixed, on both rtk versions.

Basically, it looks like draft updates aren't being correctly applied under cases with nested array data.

For example, I have this optimistic update group:

const markNotificationReadInCache = ({
  id,
  uid,
}: {
  id: string;
  uid: string;
}) => {
   const notificationListPatch = notificationsApi.util.updateQueryData(
    "getNotifications",
    { uid },
    (draft) => {
      for (const page of draft.pages) {
        const notif = page.notifications.find((n) => n.id === id);
        if (notif) {
          notif.readAt = Date.now();
          break;
        }
      }
    },
  );
  const userPatch = usersApi.util.updateQueryData(
    "getUser",
    { uid },
    (draft) => {
      draft.unreadNotificationsCount = Math.max(
        0,
        draft.unreadNotificationsCount - 1,
      );
    },
  );
  return [notificationListPatch, userPatch];
};

In this patch group, the userPatch is working as expected. The notificationListPatch, however, isn't, and the notifications aren't getting updated.

I have a few other cases where this same thing is happening.

For example, I have this scenario, where I optimistically patch comments in a discussion thread:

  const patchThreads = threadsApi.util.updateQueryData(
    "getThreads",
    {
      id: thread.post.id,
      uid: thread.post.uid,
    },
    (draft) => {
      const foundThread = draft.find((t) => t.id === thread.id);
      if (foundThread) {
        foundThread.numReplies += 1;
        foundThread.replies.push(optimisticThreadReply);
      }
    },
  );

Here, something very strange is occurs: the top-level array, (draft itself) gets an item added to it, rather than the nested entry.

---

Further, this only occurs in my production application. I've isolated this to NODE_ENV==="production", rather than something else happening in my CD pipeline.

Taking a quick look into the shipped immer.mjs, there do look to be some code branches based off of that, but this is pretty out of my depth of debugging now.

---

TLDR:

• Immer 10 -> 11 breaks array draft updates in updateQueryData
• Occurs in all recent versions of rtk
• only occurs when NODE_ENV===production
• force resolving immer version to 10.0.3 seems to fix this for me, though there may be a compatibility issue with the major version change that'll bite...

[JacobJaffe]

FWIW, I confirmed this bug occurs on both immer 11.0.0, and 11.0.1.

[markerikson]

Interesting. I'd assume this is probably about the reimplementation of the patch generation logic.

Can you capture the generated patch action contents and post them? (maybe even diff them to see what's changing?) Also, any chance of an actual repro or further endpoint/type definitions?

Technically this is an Immer bug, but we can run with it here and then file things on the Immer side later.

[markerikson]

I took a stab at reproducing this on the Immer side, and can't. If you can provide a repro or more details I can try to look further.

[chrisnojima]

@markerikson Hi. I'm running into an issue which I believe may be related (if not I can open a new issue). I don't use redux anymore (was a long time user before) but i've been following your great work on the optimizations. I made a little example repro here : https://github.com/keybase/client/pull/28706/changes you can just grab that folder or the diff (don't need the parent repo/folder). In immer 10 it works fine but in 11 it crashes out w/ the revoked proxy error

[JacobJaffe]

Working on creating a reproduction of this. The combination of variables is making it tricky, and my production app is running on 10.0.3 as a workaround for now.

[rossmartin]

We are experiencing this same issue using 2.11.2 and it only happens in a release build of the react native app. If we downgrade to 2.9.0 the issue is gone, which makes sense because that is using immer 10.

It's only happening when NODE_ENV is production. Here's a simple example of an optimistic update that doesn't work for us:

async onQueryStarted(params, { dispatch, queryFulfilled }) {
  try {
    const result = await queryFulfilled;

    if (!result?.data?.response?.photoGroupId) {
      return;
    }

    dispatch(
      photoGroupApi.util.updateQueryData('photoGroups', undefined, draft => {
        draft.photoGroups.unshift({ ...result.data.response, count: 0 });
      })
    );
  } catch {
    // no-op
  }
},

[JacobJaffe]

Worth noting this is also happening to me in a React Native environment. I've seen it on the iOS side, haven't confirmed if also happening for Android environments. (Could matter if this is a polyfill issue).

@rossmartin are you able to reproduce this by overriding NODE_ENV when running a development version of your app? I've been struggling to narrow in on a minimal reproduction because this because I can't consistently reproduce it in a dev mode iteration.

[markerikson]

Trying to recreate these myself again.

I think I've recreated @chrisnojima 's issue with Zustand, Immer, and Map/Set. I still can't recreate the RTKQ optimistic updates issue.

[rossmartin]

I made a simple expo snack that I think reproduces the optimistic update issue - https://snack.expo.dev/@gnarbucks/rtk-query-optimistic-update-issue. It can be repro'd there on any platform and the node env is production.

@JacobJaffe I can reproduce this on both ios/android in a release build. I didn't see it happen in a non-release build on either platform but didn't test much. It didn't seem to happen when running against the metro bundler and when the node env was development. I found this issue here and went ahead and downgraded back to 2.9.0

Hopefully that expo snack can help narrow this down with the optimistic update issue. Thank you both for the help on this!

[JacobJaffe]

@rossmartin super helpful to edit the snack for keeping the production env. Thank you!

Here's a modified version of the snack, to view the patch action contents (with the quickest, hackiest workaround for a lack of logging in production): https://snack.expo.dev/@jacobjaffe/a355a5

You can go into package.json and change the rtk version between 2.11.2 and 2.9.0 to see the difference.

on 2.11.2 (broken):

{
  "op": "add",
  "path": [
    0
  ],
  "value": {
    "id": "6Xna9j1k77dfw3ueIXFMD",
    "label": "Item 0"
  }
}

on 2.9.0 (working):

{
  "op": "add",
  "path": [
    "lists",
    0,
    "items",
    0
  ],
  "value": {
    "id": "_nKnLLYGpTi1u6x7cG2ip",
    "label": "Item 0"
  }
}

@markerikson getting a little closer... but I think this confirms the patch action contents are different.

--

edit: updated snack to work on web also

[markerikson]

Bizarrely, I do see the seemingly-broken patch if I run the snack on the web.

However, I do not see it if I download the snack, install deps with PNPM, and then run it in either dev or prod mode.

This is really weird.

[markerikson]

Is it at all possible that this is a bundler/minifier issue somehow?

[markerikson]

Yeahhhhh, it seems like that might be the case.

I've just compared 3 different minified versions:

• Working: the Immer section of the prod bundle from building the snack locally
• Broken: the Immer section of the prod bundle from running the snack on the web
• ?????: the immer.production.mjs file we ship in Immer

Here's the relevant original source:

// function createProxy()
	state.key_ = key


// Patches plugin
	function getPath(state: ImmerState, path: PatchPath = []): PatchPath | null {
		// Step 1: Check if state has a stored key
		if ("key_" in state && state.key_ !== undefined) {
			// Step 2: Validate the key is still valid in parent

			const parentCopy = state.parent_!.copy_ ?? state.parent_!.base_
			const proxyDraft = getProxyDraft(get(parentCopy, state.key_!))
			const valueAtKey = get(parentCopy, state.key_!)

Note the use of "key_" as a literal, vs the field name accesses.

Here's the code from the working local prod build:

// function createProxy()
      s.key_ = r,

// Patches plugin
          getPath: function e(t, n=[]) {
              if ("key_"in t && void 0 !== t.key_) {
                  const e = t.parent_.copy_ ?? t.parent_.base_
                    , r = C(M(e, t.key_))
                    , o = M(e, t.key_);

Note that preserves key_ as a field name.

But, here's the broken snack lines:

// function createProxy()
			(s.y = n),

// Patches plugin
			getPath: function t(r) {
				var n =
					arguments.length > 1 && void 0 !== arguments[1] ? arguments[1] : []
				if ("key_" in r && void 0 !== r.y) {
					var i,
						a = null != (i = r.i.e) ? i : r.i.t,
						u = B(F(a, r.y)),
						o = F(a, r.y)

Note that key_ was not preserved as field name here... which means that the if ("key_" in r check will fail!

Interestingly, here's the pre-minified immer.production.mjs file we ship:

// function createProxy()
		(i.y = n),

// Patches plugin
function tt() {
	function t(c, P = []) {
		if ("key_" in c && c.y !== void 0) {
			let m = c.i.e ?? c.i.t,
				x = Te(U(m, c.y)),
				A = U(m, c.y)

Note that this also does not keep key_.... which means it's probably also broken!

Seems like just removing the if ("key_" in check might fix this.

That's separate from the Set issue.

Let me go file a couple Immer issues just to keep track of these.

[markerikson]

Okay, filed https://github.com/immerjs/immer/issues/1199 for the array issue, and https://github.com/immerjs/immer/issues/1200 for the Set issue.

[markerikson]

Awright, looks like this was indeed two separate issues:

• Array patches not working: Immer's ESBuild config tries to minify "private" fields like key_, but the logic for patch path lookup had a if ("key_" in state) check. That check failed when key_ got renamed.
• Map/Set handling: the repro takes an existing DraftSet instance, puts it in a new plain object, and puts the plain object into a DraftMap. That effectively cuts the chain of assignments, and we never tried to fix up the DraftSet in the plain object. We already have a handleCrossReference method that does this, it just wasn't being called inside of DraftMap.set().

Got them both fixed locally, just need to make sure there's simplified tests that cover these cases.

[markerikson]

@JacobJaffe @rossmartin @chrisnojima I've got a PR up that ought to fix these issues:

• https://github.com/immerjs/immer/pull/1201

We still don't have PR preview builds set up for Immer, but you could try:

• clone the Immer repo
• check out that branch
• run yarn && yarn build
• use yalc to "publish" the build locally (npx yalc in the Immer repo)
• use yalc to use the build in your project (npx yalc add immer in your project repo, then reinstall with your project manager of choice)

[markerikson]

The fixes should be out in Immer 11.1.2 / 11.1.3!

[JacobJaffe]

Great, thank you so much for digging into this issue!

[markerikson]

Well, I ~~broke~~ shipped it, I gotta fix it :)

but yeah, thanks for the repros! Really needed those to be able to figure out what was going on here, especially given the bizarre obscurity of this issue.