fix(react-router-dom): Fix usePrompt invalid blocker state transition

[louis-young]

PR for #10489.

This pull request fixes the usePrompt invalid blocker state transition invariant exception.

Essentially, when navigating, if we should block navigation, we update the blocker for the derived blocker key so that it's in a "blocked" state. In the proceed function for this blocker, we then update the blocker so that it's in a "proceeding" state. In the reset function for this blocker, we reset the blocker to the IDLE_BLOCKER (which results in an "unblocked" state).

In the updateBlocker function, we receive the blocker key as a parameter and update it. What was happening here is that by the time we came to proceed the blocker, it was in a "proceeding" state, causing the invariant to throw an exception according to the state-machine-like logic (logically, as this makes no sense).

The blocker was in this state because in the usePrompt hook, there are two effects: one that resets the blocker when the when parameter changes, and one that proceeds the blocker after confirmation. These two effects were essentially fighting each other; the first effect was resetting the blocker just before the second effect attempted to proceed it, leading to the unexpected state transition.

What I've done (after reasoning about this with brophdawg11, thanks for your time and expertise Matt) is both reversed the definition order of the effects so that we first attempt to proceed a blocker when appropriate, and also removed the setTimeout wrapping the call to blocker.proceed as I think this defers the execution of the function, which probably isn't what we want here.

This appears to fix the problem 🎉

Changes

• Remove the setTimeout wrapping blocker.proceed. This defers the execution of the function, but I don't think that's what we want here.
• Reorder the useEffects so that we attempt to proceed first when appropriate and prevent resetting a blocker that we're about to proceed.

Notes

• If anyone knows exactly why the setTimeout wrapping blocker.proceed was required, or why anything I've done here doesn't sound like a good idea then please let me know 🙂

[changeset-bot]

⚠️ No Changeset found

Latest commit: d470e99

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[remix-cla-bot]

Hi @louis-young,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

[remix-cla-bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

[timdorr]

Can we add a test as well?

[louis-young]

Can we add a test as well?

Hi 👋

Thanks for taking a look.

I looked at the test coverage when making the change and couldn't find any tests for usePrompt at all. I assumed this was an intentional decision but is this something that we want to change? I had a look at https://github.com/remix-run/react-router/blob/main/decisions/0001-use-blocker.md#decision but couldn't find any explicit decisions relating to test coverage (or lack thereof).

I'm more than happy to write the tests 🙂

Thanks!

[brophdawg11]

To be frank, useBlocker is the bread and butter (and the preferred approach since it avoids the cross-browser differences in window.confirm behavior) and that's where we added the tests. We initially weren't planning to include usePrompt because it's non-deterministic and such a small hook that could easily be done in userland, but since we had a working example, rather than just put it in the docs and say "here, copy this hook into your app" - we added it in with unstable_ but never wrote any tests around it.

That said - more tests is never a bad thing so if they're not too much of a pain to implement (along with mocking window.confirm via jest/jsdom) it would be great to get at least one in for this specific problematic use case, and then we can add more later on. But if it proves to be a huge pain to get the mocking working then don't worry about it. For some things that require a real browser we've found it can be easier to write E2E tests via Playwright in Remix

[louis-young]

To be frank, useBlocker is the bread and butter (and the preferred approach since it avoids the cross-browser differences in window.confirm behavior) and that's where we added the tests. We initially weren't planning to include usePrompt because it's non-deterministic and such a small hook that could easily be done in userland, but since we had a working example, rather than just put it in the docs and say "here, copy this hook into your app" - we added it in with unstable_ but never wrote any tests around it.

That said - more tests is never a bad thing so if they're not too much of a pain to implement (along with mocking window.confirm via jest/jsdom) it would be great to get at least one in for this specific problematic use case, and then we can add more later on. But if it proves to be a huge pain to get the mocking working then don't worry about it. For some things that require a real browser we've found it can be easier to write E2E tests via Playwright in Remix

I agree with all of this. This seems like a reasonable and pragmatic approach 👍

I'll take a look at writing some tests and go from there 🙂

[louis-young]

I've written a suite of unit tests for usePrompt 🙂

A few things to note:

1. I used a browser router in an attempt to make the tests slightly more realistic. This required pushing onto the history stack after each test to clean up properly between tests.
2. We don't appear to be using @testing-library/user-event, so I've just used the click property of the DOM elements and surrounded those calls in an act call.
3. I decided against making abstractions here like a createTestBrowserRouter function to create the browser router and define the routes. I also didn't bother variablising things like the route titles, paths, and link text. There aren't many tests here and it didn't seem like a worthwhile thing to do.

I didn't actually write a test case for the specific use case that surfaced this regression as it didn't fit very nicely into the suite and would essentially be checking that we don't introduce this specific regression again. Is this something that we want to do? If so, is there a convention defined in the codebase for writing test cases like this? I'm more than happy to extend the test suite.

What do you think?

[brophdawg11]

These tests are great @louis-young! Thank you! I made some minor updates in d470e99 - added getWindowImpl to get a working history and consolidated the tests to just use inline Components instead of reusing PromptRoute/ArbitraryRoute.

[louis-young]

These tests are great @louis-young! Thank you! I made some minor updates in d470e99 - added getWindowImpl to get a working history and consolidated the tests to just use inline Components instead of reusing PromptRoute/ArbitraryRoute.

Thank you! I had a look and the tweaks look great, nice one. I forgot that fireEvent shipped with @testing-library/react too, well spotted!

Thanks for putting the changeset together too.

This looks good to me. Thanks for your help with this 🎉

[lennerd]

Ah, wait. I think the setTimeout is still needed! I was able to work around this but believe the bug to be related to using navigate from useNavigate with numeric values (e.g. for moving one step back in the history with -1).

In Arc (a Chromium based browser) the path is changing to the previous path but switching back to the current one, when the confirm dialog was closed with OK.

Can you reproduce this? Or should I try to come up with a reproducable Sandbox?

[louis-young]

Ah, wait. I think the setTimeout is still needed! I was able to work around this but believe the bug to be related to using navigate from useNavigate with numeric values (e.g. for moving one step back in the history with -1).

In Arc (a Chromium based browser) the path is changing to the previous path but switching back to the current one, when the confirm dialog was closed with OK.

Can you reproduce this? Or should I try to come up with a reproducable Sandbox?

Hi 👋

Please could you open an issue so that we can track this better?

If you could provide a minimal reproducible example then that'd be really helpful.

Thanks.

[brophdawg11]

I think I was able to quickly reproduce this, so I filed #10714.

@lennerd - please add any reproductions you are able to come up with to that issue 🙌