Repository navigation
Questions about lifecyle and the triggering of event #3443
Description
Activity
Hi @remiHau, if I am understanding the issue correctly, I think I would recommend using the
beforeShowPromiseof the next step to do whatever cleanup you need from the previous step. Would that work?If I understand the code correctly,
beforeShowPromiseof the next step would not work if it is the last step of the tour because it wouldn't be triggered because it is called only onshowfunction and not oncomplete. We can duplicate the cleanUp code to be called in two different event, but I think it's a bit more logical to have the step responsible of his own cleanUp, no?Hello,
Do you have any news on this issue? We created a PR to show what we had in mind, but let know if there is issues with the changes or if we should implement a different solution
@remiHau — thanks for the detailed write-up, and sorry for the wait. Your diagnosis is right, and this is a bug on our end. Here's what's actually going on.
Why
destroyfires on every re-showStep.elhas three states:undefined(never shown),null(destroyed), or an element (mounted)._setupElements()rebuilds the element on every show and tears down whatever was there first — but it does that by calling the publicdestroy(), which emits the event:_setupElements() { if (!isUndefined(this.el)) { this.destroy(); // <- emits `destroy` } this.el = this._createTooltipContent(); // ... }
So the second time a step is shown, the order is
before-show→destroy→show. In your casebeforeShowPromiseopens the menu, then thatdestroycloses it again a moment later — exactly the back → next behavior you described. You're also right that this makeshidevsdestroyfeel pointless as a distinction today.One small correction to the archaeology: the guard itself predates the commit you found by a couple of months (#430, July 2019). What 0fff410 (#583) changed was making
_show()call_setupElements()unconditionally, which is what started exercising that guard on every show.About your PR (#3455)
Thanks for opening it — but I don't think it fixes the symptom you reported, and it's worth saying why.
hide()never nullsel; it only setsel.hidden = true. So on a plain back → next,this.elis still anHTMLElement,!isUndefined(el) && !isNull(el)istrue, anddestroy()fires exactly as before. TheisNullcheck only helps in the narrower case where you've already calleddestroy()yourself from yourhidehandler — so it makes your workaround viable rather than removing the need for one.To answer your question directly (change the condition to null-or-undefined, or have
destroy()setel = undefined): neither, in the end. Both leave a public lifecycle event firing from a private code path. Theundefined/nullsplit also carries documented meaning ongetElement()— "undefined if it has never been shown, null if it has been destroyed" — so we'd rather not collapse the two.What we're doing instead
There's already an internal teardown that does everything
destroy()does, minus the event:/** * Internal cleanup that tears down the tooltip, component, and DOM element * without emitting the public "destroy" event. * @private */ _teardownElements() { /* ... */ }
_setupElements()should have been using that. With the switch,destroymeans one thing — this step is gone for good — and it fires fromstep.destroy(), fromtour.removeStep(id), and once per step from_done()when the tour completes or is cancelled.That gives you what you were after with no workaround: open the menu in
beforeShowPromise, close it inwhen: { destroy }. It fires once, and it still runs for the last step, because completing or cancelling the tour destroys every step. Your point about the step owning its own cleanup is fair — duplicating it into the next step'sbeforeShowPromiseshouldn't be necessary.Two things had to ride along, which is why I'd rather supersede #3455 than merge it:
advanceOn's only unbind path wasstep.on('destroy', …), so removing the per-show event would have leaked a DOM listener on every show.bindAdvancenow returns a cleanup function that teardown calls — which also closes out an oldTODO: this should also bind/unbind on show/hide._teardownElements()wasn't idempotent. It clears the stored tabindex map but never resetstarget, so running it twice permanently dropped a target's originaltabindexvalue. That was already reachable throughupdateStepOptions(). Guarding onisHTMLElement(this.el)covers all threeelstates at once and fixes it.
The lifecycle is now documented in the usage guide too, since "which events fire when" clearly wasn't discoverable.
PR: #3474
Your write-up is what made this quick to pin down — thanks for taking the time on it, and for offering to fix it yourself.
- added a commit that references this issue
on Aug 13, 2026
Hi,
We are planning to use shepherd to showcase our application because it seems to be a really good tour application. But before purchasing the license , we made some tests to see if we were able to do everything we needed and we met an issue, maybe you will be able to explain us what we are doing wrong or the logic behind it.
First of all, to explain a bit the context, we are showcasing the application and it works really well for simple case where the htmlElement to showcase is available in the DOM, but for one specific case we need to perform some actions before and after the step (in this specific case, open a three-dots menu before the step and close it after it). So to do that, we plugged the application with the hook event of the step (beforeShowPromise, to open the menu and hide/destroy to close the menu). We encountered an issue where the destroyed event was called, whenever we went back to the step, which closed again the menu on a back->next action.
By searching into the code, we found out that since this commit linked to this issue, the destroy event is always called, which make the hide state a bit pointless.
So we thought calling the destroy method in the hide hook, to be sure that the element was destroyed and not be destroyed again on show, but since the same commit, the condition to call the destroy event is to check if it is undefined and not falsy as before, but the destroy method set the htmlElement to null and not to undefined so even if it is destroyed, the destroy method will be always called.
It's possible that we misunderstood the flow or some specific conditions, but do you think it would be possible to either change the condition of destruction during the show, to be null or undefined or to change the destruction method so it set the internal this.el to undefined instead?
If you don't have the time to do that, we would also be happy to create a PR to do thoses changes.
Thank you for your time