Per-block custom user confirm

[Karagoth]

I found the need to have more information available at user-confirm time than a string could provide, and also the need to customize further than only a different message. This change adds the capability to curry a custom user-confirm function when creating the block, which makes the task easy.

[fernandojbf]

#489. Ok... i'm not the only one thinking like this :D

[Karagoth]

Not really sure why the tests fail though, it's only on the Mobile Safari 8.0.0 browser and says in the log: Mobile Safari 8.0.0 (iOS 8.3.0): Executed 116 of 116 DISCONNECTED (18.156 secs / 1.617 secs) which makes me want a re-run. Unfortunately I either lack permission or knowledge of how to re-trigger the CI-tests (without creating a dummy commit or amending).

[bwhitty]

I believe that this is exactly what is needed to solve remix-run/react-router#5254 by providing a callback-like API to react-router's <Prompt /> such as one described in remix-run/react-router#4635 (comment)

RR could, inside the render callback, render a user's custom component which could (via some API) trigger the callback provided here dynamically.

[mjackson]

I don't like the fact that getUserConfirmation is bypassed for the duration of the block when using this signature. Can you please describe what you're trying to do and how the current API falls short? I'm having trouble understanding from the conversation so far and the tests you've got.

[bwhitty]

@mjackson the current way that getUserConfirmation can only be set during history object creation means, for example, that react router's usage of it is inflexible as it is only configured at the top of the app tree in <Router getUserConfirmation={...}. Being able to reach into this API outside of just the factory is needed for more advanced usage in react router. See remix-run/react-router#5254 and remix-run/react-router#4635

Allowing the passing of a function (or maybe a Promise) would enable user-space APIs to configure the block deeper in the application. Such as in the following psuedo react app which needs to save data if it's not already saved prior to allowing the route to update:

// main
<Router>
  <div>
    <Route path="/foo" component={Foo} />
    <Route path="/bar" component={Bar} />
  </div>
</Router>

// Foo.jsx
class Foo {
  componentDidMount() {
    this.props.history.block((callback) => {
      if (someDataNeedsSaving) {
        displaySomeCustomUi()

        saveSomeData()
          .then(() => callback(true)))
          .catch(() => callback(false))
      }

      callback(true)
    })
  }

  render() {
    <Link to="/bar" />
  }
}

export default withRouter(Foo)

// Bar.jsx
class Foo {
  componentDidMount() {
    this.props.history.block((callback) => {
      if (someOTHERDataNeedsSaving) {
        displaySomeDIFFERENTCustomUi()

        saveSomeOTHERData()
          .then(() => callback(true)))
          .catch(() => callback(false))
      }

      callback(true)
    })
  }

  render() {
    <Link to="/foo" />
  }
}

export default withRouter(Bar)

[Karagoth]

@mjackson My goal is to be able to use a dialog with three buttons to:

• Save a modified form then navigate away save(); callback(true);
• Discard changes and navigate away callback(true)
• Stay callback(false)

Since the getUserConfirmation is created very early, you have inject pre-callback functions by possibly storing them in global variables, that then need to be updated anytime a React-Router is mounted and unmounted. I haven't tried this solution but I wouldn't make it this way unless I had to.

[mjackson]

I think a per-block confirmation function is probably the right way to go.

A little history: I created the getUserConfirmation function when we supported blocking the beforeunload event. In the event handler, you used to be able to assign event.returnValue to be a string that the browser would show in a modal dialog to the user just before closing the window. There was no way to show custom UI in that situation. But browsers decided this feature was a security risk and removed it, so the whole API is pointless now.

So I'd definitely welcome a change to this API :)

However, @Karagoth I'm a little confused about the API you're going with here. Instead of

history.block((location, action) => (callback) => ...)

why don't we just go with

history.block((location, action, callback) => ...)

?

[lourd]

Can users register multiple blocker functions?

let unblock1 = history.block((location, action, callback) => ...)
let unblock2 = history.block((location, action, callback) => ...)

[mjackson]

No, they can't currently. I think that's part of the problem this PR is trying to address.

[lourd]

Apologies, I wasn't clear — I was asking about the proposed API. Would you allow for the history.block API to support being called multiple times?

[mjackson]

Yes, I think so. I need to go through the use cases.

[Karagoth]

@mjackson glad to hear back from you.

The suggested history.block((location, action) => (callback) => ...) API was mostly because it expanded the current functionality with no breaking changes (unless someone besides me returns a function in the prompt, why would anyone do that?).

In principal I have no issue with the history.block((location, action, callback) => ...) API. But if we want non-breaking changes, what the prompt function returns here needs to be handled carefully, since the current implementation does the callback one way or the other, no matter what you return.

I will ponder it and see if I can find some time in the weekend to make the change to make sure the suggested API still solves my problem (it likely will).

[aselbie]

Anything I can do to help move this along? Seems like an elegant solution as long as the user is comfortable with a curried function.

[mjackson]

@aselbie You can make a PR with tests that pass and a bunch of clearly defined use cases. The lack of use cases here is what's stalling this one for me.

I'm closing this PR since it's stalled. But I'm definitely open to having a per-block confirmation function. Just needs someone who's willing to do the work to push it through.

[lourd]

The use cases were fleshed out in #493, most fully in this comment.

The only material differences between this proposal is backwards compatibility. This keeps the name "block", and enables the desired API through a new argument type so no one has to change their code.

You weren't for that proposal, I didn't think you'd be for this one either. I still believe an async pre-change hook would be a positive addition to the library. The router docs for scroll restoration have described the shortcoming for a long time.