americanexpress / react-albus

✨ React component library for building declarative multi-step flows.
Apache License 2.0
1.1k stars 89 forks source link

Calling next method in onNext handler causes infinite loop #42

Closed benhodgson87 closed 3 years ago

benhodgson87 commented 5 years ago
<Wizard onNext={({ step, next }) => {
  if (step.id) {
    console.log('onNext called')
    next()
  }
}}>

Got Albus working fine when running without an onNext handler, but now need to add an event that will be called whenever the user successfully navigates to the next step, whilst still navigating them to the next step.

Unfortunately the above example code results in an infinite loop. onNext is recursively called until the browser kills the process, presumably as calling next results in onNext being called, which in turn calls next again.

I've managed to get around this by working out the next step and calling push instead, however this feels less than ideal.

<Wizard
  onNext={({ step, steps, push }) => {
    if (step.id) {
      console.log('onNext called')
      push(
        steps[steps.findIndex(el => el.id === step.id) + 1].id
      )
    }
  }}
>

Is this a bug, or expected behaviour? Is there a better way to call a function upon successful step navigation, without having to call next to maintain the default navigation behaviour, which results in this issue?

asutedja commented 4 years ago

Sorry for the very late reply.

Got Albus working fine when running without an onNext handler, but now need to add an event that will be called whenever the user successfully navigates to the next step, whilst still navigating them to the next step.

If next is being called, that means the user should be moving to the next step anyway. Are you trying to navigate the user to a step other than the next step? If so, then push would make more sense to use.

benhodgson87 commented 4 years ago

The issue is that when you provide an onNext handler, it overrides the existing functionality which takes the user to the next step. It only runs the code provided to onNext. This means when you click the next button, your custom code will run, but you won't be taken to the next step. Instead you have to manually call next within your custom handler, however this is where the infinite loop is caused, as next itself calls onNext, which in turn will call next again, and so on.

Edit:

The issue appears to be here: https://github.com/americanexpress/react-albus/blob/master/src/components/Wizard.js#L96

It only makes the call to push to the next step if onNext is not set. What it should really be doing is calling the onNext prop if provided and pushing to the next step.

Edit 2:

Looking at the source code above again, I see that calling push without an argument will push to the next page by default. Not very clear from the documentation (it looks like push requires an id as an arg) but I guess in this case calling push() in the onNext handler will resolve.

Readme could possibly do with being updated to reflect the fact that onNext overrides the default behaviour, and that push is the correct method to navigate to the next step within a custom onNext function, rather than the more obvious next.


<Wizard onNext={({ push }) => {
  console.log('onNext called');
  push();
}}>
github-actions[bot] commented 4 years ago

This issue is stale because it has been open 30 days with no activity. Remove no-issue-activity label or comment or this will be closed in 5 days.

github-actions[bot] commented 4 years ago

This issue is stale because it has been open 30 days with no activity.