Skip to content

fix: forward thisArg in the array-methods plugin - #1292

Open
giaBaoJS wants to merge 1 commit into
immerjs:mainfrom
giaBaoJS:fix/array-methods-thisarg
Open

fix: forward thisArg in the array-methods plugin#1292
giaBaoJS wants to merge 1 commit into
immerjs:mainfrom
giaBaoJS:fix/array-methods-thisarg

Conversation

@giaBaoJS

Copy link
Copy Markdown

Problem

With enableArrayMethods() enabled, filter, find and findLast ignore the optional thisArg argument, so any predicate that reads this throws at runtime.

import {produce, enableArrayMethods} from "immer"
enableArrayMethods()

const base = {items: [1, 2, 3, 4, 5]}
const ctx = {min: 3}

base.items.filter(function (x) {
	return x >= this.min
}, ctx) // [3, 4, 5]

produce(base, draft => {
	draft.items.filter(function (x) {
		return x >= this.min
	}, ctx)
	// TypeError: Cannot read properties of undefined (reading 'min')
})

find and findLast fail the same way. With the plugin disabled all three return the native result, so enabling the plugin for its performance benefit silently breaks working code. The usual shape is a method passing its own instance through:

class Cart {
	expensiveItems(draft) {
		return draft.items.filter(function (i) {
			return i.price > this.limit
		}, this)
	}
}

Cause

src/plugins/arrayMethods.ts hand rolls these three methods for the proxy avoidance optimization, and invokes the predicate as a bare call, so args[1] is never read:

  • filter: predicate read at line 449, called at line 454
  • find / findLast: predicate read at line 464, called at line 470

The other predicate taking methods are unaffected precisely because they are not hand rolled: findIndex, findLastIndex, some and every fall through to source[method](...args) at line 500, which forwards thisArg natively. So the plugin already honours thisArg for half of its predicate methods. The Map/Set plugin does it explicitly, at src/plugins/mapset.ts:113 and :309:

cb.call(thisArg, this.get(key), key, this)

Spec

ECMA-262, 15th edition (ES2024), https://262.ecma-international.org/15.0/

23.1.3.8 Array.prototype.filter ( callbackfn [ , thisArg ] ), step 7.c.ii:

ii. Let selected be ToBoolean(? Call(callbackfn, thisArg, « kValue, 𝔽(k), O »)).

and Note 1 of the same section:

If a thisArg parameter is provided, it will be used as the this value for each invocation of callbackfn. If it is not provided, undefined is used instead.

23.1.3.12.1 FindViaPredicate ( O, len, direction, predicate, thisArg ), which Array.prototype.find (23.1.3.9) and Array.prototype.findLast (23.1.3.11) both delegate to:

thisArg will be used as the this value for each invocation of predicate.

and its step 4.d:

d. Let testResult be ? Call(predicate, thisArg, « kValue, 𝔽(k), O »).

Fix

Read args[1] and invoke the predicate through predicate.call(thisArg, ...) in both branches. Four lines. When thisArg is omitted it is undefined, which is exactly what a bare call already produced, so the common path is unchanged.

The third argument stays source (the base array) rather than the draft. That is the plugin's existing documented behaviour, "callbacks receive base values", and is out of scope here.

Verification

  • yarn vitest run: 3764 passed, 8 skipped before, 3809 passed, 8 skipped after. The 45 extra results are 5 new tests times the 9 harness variants in __tests__/base.js.
  • Counterfactual: reverting only src/plugins/arrayMethods.ts while keeping the tests gives 3 failures, all TypeError: Cannot read properties of undefined, with stacks pointing at arrayMethods.ts:454 and arrayMethods.ts:470. All 3 are in the array-plugin=true variant only; the 8 plugin disabled variants stay green, which is what pins this as plugin specific.
  • Two of the 5 new tests are guards rather than regressions, and pass on baseline by design: one pins that findIndex, findLastIndex, some and every still receive thisArg, and one pins that an omitted thisArg leaves this undefined per Note 1 above.
  • yarn test:build (against the built bundle): 3258 passed, 8 skipped.
  • prettier --check src/plugins/arrayMethods.ts is clean, and the block added to __tests__/base.js is prettier clean as well. I deliberately left the 5 pre-existing function() occurrences elsewhere in that file untouched, so the test diff is a pure insertion with no reformatting noise.
  • yarn test:flow could not run locally: flow-bin ships an x86-64 binary and fails to spawn on arm64 with Error: spawn Unknown system error -86, before reading any file. It is unaffected either way, since this change touches no Flow types.

One related defect I deliberately left out

Same function, same spec section: filter also skips step 7.b, Let kPresent be ? HasProperty(O, Pk), so it visits array holes.

const sparse = {items: [1, , 3]}
sparse.items.filter(() => true) // [1, 3], predicate called twice
produce(sparse, d => d.items.filter(() => true)) // [1, undefined, 3], called three times

I left it out because the boundary is not the plugin's. Immer's own shallowCopy densifies sparse arrays, so once a draft has been modified the plugin disabled path visits holes too:

produce({items: [1, , 3]}, d => {
	d.items.filter(() => true) // plugin off: length 2
	d.items.push(9)
	d.items.filter(() => true) // plugin off: length 4, the hole is gone
})

Fixing the plugin alone would make it match the plugin disabled path, but that path is itself lossy, so the outcome would still not be spec conformant and the real question is whether Immer wants to preserve holes at all. That reads like a decision for you rather than something to fold into a crash fix. Happy to open a follow up if you want the plugin brought in line with core regardless.

The plugin hand-rolls filter, find and findLast and invoked the
predicate as a bare call, dropping the optional thisArg argument. Any
predicate reading `this` threw a TypeError once enableArrayMethods()
was on.

findIndex, findLastIndex, some and every were unaffected: they fall
through to the native method, which forwards thisArg itself. The
Map/Set plugin already does the same with cb.call(thisArg, ...).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant