Skip to content

Fix push() dropping valid items after an invalid/fctn entry, and remo… - #177

Open
kenmckell wants to merge 1 commit into
adobe:masterfrom
kenmckell:fix/push-and-clonedeep-bugs
Open

Fix push() dropping valid items after an invalid/fctn entry, and remo…#177
kenmckell wants to merge 1 commit into
adobe:masterfrom
kenmckell:fix/push-and-clonedeep-bugs

Conversation

@kenmckell

Copy link
Copy Markdown

This PR fixes two related bugs in push(), both reproducible on unmodified master.

1. Multi-item push() calls could silently drop valid items.

push() tracked which items should end up in the array by deleting invalid/function/listener entries out of the same array holding the original arguments, then only checked whether index 0 of that array survived before deciding whether to add anything at all:

if (filteredArguments[0]) {
  return Array.prototype.push.apply(this, filteredArguments);
}

Since delete leaves a hole rather than shifting later items down, if the first argument in a call was invalid or a function, every other valid item in that same call was silently excluded from the array — even though its data was still merged into state and listeners still fired correctly.

2. Pushing a listener object directly ({ on: ..., handler: ... }) registered nothing.

The LISTENER_ON/LISTENER_OFF case only removed the item from the array-bound arguments; it never called _processItem(), so registration/removal never actually happened. Only addEventListener()/removeEventListener() worked.

Fix: push() now builds a clean, independent list of items that should be visible, and calls _processItem() for every recognized item type, including listeners. Also removed dead code in cloneDeepWith (src/utils/mergeWith.js) that built a manual recursive clone which was always immediately discarded in favor of structuredClone.

Testing: I wasn't able to get the full local Jest suite to run cleanly in my environment — events.test.js and several other suites fail with RangeError: Maximum call stack size exceeded inside @ungap/structured-clone. I confirmed this is pre-existing and unrelated to this change (reproducible identically on unmodified master). Per that polyfill's own documentation, this specific error occurs when it's attached to the global scope without checking for a native structuredClone implementation first — likely a version mismatch between an older pinned Jest dependency and newer Node versions (tested on Node v22.22.1) that already provide structuredClone natively.

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