Fix skipping of event handlers

[bausmeier]

Event handlers registered after a once handler were being silently skipped, causing unpredictable behaviour in tour interactions. This occurred because modifying an array during iteration is unsafe.

Making a copy of the bindings array before iteration prevents these handers from being skipped.

[vercel]

@bausmeier is attempting to deploy a commit to the shipshapecode Team on Vercel.

A member of the Team first needs to authorize it.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
shepherd-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 27, 2025 6:01pm

[bausmeier]

Another approach I considered was to iterate the bindings in reverse order with a for loop. This would allow us to avoid making a copy of the array, but could have an observable impact for existing users of the package. I thought it would be best to avoid what could be considered a breaking change.

[RobbieTheWagner]

@bausmeier what if instead of creating a copy, we made a new array like const listenersToRemove = [] and then pushed the ones to remove into that, then after the forEach is done remove them all?

[RobbieTheWagner]

@bausmeier sorry for the delay in reviewing this! Could you please explain the exact problem a bit more?

[bausmeier]

@RobbieTheWagner Sure thing. Sorry I didn't explain more clearly in the first place.

In our project, we need to listen for the same event in multiple places (analytics and business logic), and in the particular case where this problem arises we're listening for the complete event using once in one place and on in another.

As an illustrative example, we call .once('complete', recordCompletion) before .on('complete', sendAnalytics) on the same tour and expect that both of the listeners will be called when the tour completes, but what we're seeing is that recordCompletion is called and sendAnalytics isn't. If we use on in both places or call .on('complete', sendAnalytics) before .once('complete', recordCompletion) then both listeners are called as expected, but neither of those workarounds is feasible for us.

Hope that clarifies things. If you need an example to reproduce this I can try and put something together, but I was able to write a failing test so didn't think that was necessary.

[chuckcarpenter]

closed in favor of #3476

[chuckcarpenter]

Thanks for this @bausmeier, and apologies it sat so long. The bug is real and your diagnosis was right — trigger() was mutating this.bindings[event] while forEach walked it, so removing a spent once binding shifted every later binding down one and the handler that moved into the vacated slot got skipped.

I've opened #3476 to land the fix, with you as co-author on the commit.

Two reasons I didn't just rebase this branch:

Mechanically, it forked at 2025-05-06 and is ~160 commits behind. test/unit/evented.spec.js has since moved to shepherd.js/test/unit/evented.spec.js and the suite moved from Jest to Vitest, so the test needed relocating and jest.fn() → vi.fn(). "Allow edits from maintainers" is off on your fork, so I couldn't push a rebase here either.

More substantively, iterating over a copy alone doesn't cover every case. index becomes an index into the copy while splice still targets the live array, so the indexes drift as soon as two or more once handlers are registered for the same event — all of them fire, but a spent binding survives and fires again on the next trigger. #3476 removes once bindings by identity (indexOf) instead. It also fixes the same splice-during-iteration bug in off(), which left one binding behind when the same handler had been registered twice.

Your test case is in there as a regression test, alongside three more covering the cases above.

[bausmeier]

Thanks for the update @chuckcarpenter. Makes sense.