FIX : prevent tour from closing on left arrow key at first step

[iRittikSharma]

• Prevented the tour from being destroyed when pressing the left arrow key on the first step.

• Added checks in:

  • drive() – exits early if stepIndex < 0.
  • movePrevious() – exits if activeIndex === 0.

• Didn't add the check in handleArrowLeft() to allow client-defined onPrevClick callbacks to run as expected.

Fixes #564

[jarikmarwede]

How about throwing errors if an invalid action is triggered instead of silently ignoring it? I think it would be more helpful for debugging if movePrevious, moveNext, moveTo and drive all threw errors instead of calling destroy or just returning.

For moveTo and drive RangeError seems like it fits. For movePrevious and moveNext I don't think there is a core error that could be used so we could create a custom one like InvalidDriverActionError.

[iRittikSharma]

That's a great suggestion — I agree that throwing errors can be helpful for debugging in many cases. However, for the moveNext() function specifically, I think we should not throw an error or add a check like steps.length >= currentIndex. Here's why:

• The moveNext() function relies on checking steps[currentIndex] to determine whether the tour has reached the end.
• If we prevent currentIndex from ever reaching steps.length, the function will never recognize that the tour is over, and the tour will not be destroyed as intended.
• Since drive() and moveTo() also rely on this logic to end the tour, throwing an error when currentIndex >= steps.length would break the intended session cleanup flow.

[jarikmarwede]

I think I understand what you mean. Of course, we still have to implement destroying the tour if the right arrow key is pressed on the last step. However, I would suggest handling this logic outside moveNext and drive in the event handlers directly or creating wrapper functions for this.

This would be cleaner because moveNext and drive are part of the public API, so they can be used by the users of the library directly and in my opinion should do error handling. The event handlers however are not part of the API and therefore seem like a better place for this logic.

In my opinion, if I call the moveNext function on my own Driver object, I don't expect it to destroy itself if there is no next step. I would only expect it to do that if I explicitly call destroy. However, I did not design this API and maybe Kamran just wants it to be this way. In that case, the way in which you have solved the problem makes sense.

[iRittikSharma]

Added isDriver(this) check in moveNext() to differentiate internal vs. client calls — throws error when called by client with no next step, silently destroys tour otherwise.

[iRittikSharma]

Hi @jarikmarwede ,
I have implemented the suggested changes.
Just wanted to check if there's any update or if anything else is needed from my side.
Thanks!

[jarikmarwede]

Hi @jarikmarwede , I have implemented the suggested changes. Just wanted to check if there's any update or if anything else is needed from my side. Thanks!

Hey, sorry for taking so long to reply.
I think the marker functionality you added to differentiate between internal and external calls is a bit over complicated. I think it would be cleaner to always throw the errors and just handle them in the calling functions for the internal calls.

However, I think that changing the functions in general would mean multiple breaking API changes and I don't think @kamranahmedse would merge that anytime soon. I believe it would be best to do these changes in another pull request and have created Issue #575 for discussing this further. I'm sorry I suggested adding the error handling in this pull request. If you force pushed just the first commit and leave the error handling part out of this, I think it is more likely to be merged soon.

Also, just to clarify, I am not the maintainer of this project. I just created the issue you are trying to fix with this PR.