Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

History Blocking: Fix loop rendering and usePrompt #9821

Closed
wants to merge 19 commits into from

Conversation

develra
Copy link

@develra develra commented Jan 5, 2023

Note: This should target #9709 - but the implementation changed drastically after 3545cdc - I reviewed the changes and pulled in the bits that still work well with the previous implementation.

This PR includes two major fixes for History Blocking that I needed to be able to continue with our React Router migration. Feel free to take them whole-sale or just pick out the pieces you like. I will say the usePrompt logic took me 4/5 days of iterating and thinking hard about the various scenarios. I tried to comment the code well, but let me know if you have any additional questions.

Here is the behavior usePrompt ended up with - I think it's the best I could do with my approach. To reiterate the issues for what happens if you close a previous prompt with a new back-button click:

Chrome: closes the prompt and triggers the 'shouldBlock' conditional case (window.confirm is false) BUT if it triggers another navigation that event gets dropped on the floor as there is a navigation from the back-button pending.

Safari: closes the prompt and triggers the 'shouldBlock' conditional case (same as chrome), BUT if it triggers another navigation that event gets queued and handled in addition to the navigation event from the back-button.

Firefox: stacks the prompts on top of each other and keeps previous prompts suspended. As the user clicks 'OK' or 'Cancel, the correct response is queued and then all prompts resolve when the final one closes, all navigation triggered seem to happen.

FWIW I think Chrome is the most 'buggy' implementation as it calls a function and drops it on the floor with no error or warning. Both Safari and Firefox have reasonable implementations IMO.

prompt state Chrome Firefox Safari
single prompt accepted works as expected works as expected works as expected
single prompt rejected works as expected works as expected works as expected
multi prompt accepted will cancel instead works as expected if user clicks OK on all prompts, else cancel URL is wrong until user clicks OK and then navigates to the right place
multi prompt rejected works as expected works as expected works as expected

Please pull this and play around with it. AFAICT everything is working as it should, but I could easily have missed something. I think if we do end up taking this whole-sale, it would make sense to mark usePrompt and useBlocker as unstable for the first release to work out an kinks.

Lemme know if you have any questions - I'm both here and on Discord,
Develra

@changeset-bot
Copy link

changeset-bot bot commented Jan 5, 2023

🦋 Changeset detected

Latest commit: a40985c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Minor
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 5, 2023

Hi @develra,

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

@develra develra force-pushed the history-blocking-fixes branch 2 times, most recently from c6d99c1 to d651f91 Compare January 9, 2023 16:54
@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 9, 2023

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

- Implement logic for usePrompt to work in most cases on all major
  browsers.
- Fixes an issue with useBlocker that was causing it to endlessly loop.
- Added a few notes to DEVELOPMENT.md about how to work on react-router.
@chaance
Copy link
Collaborator

chaance commented Jan 13, 2023

@develra and I discussed one-off already, but sharing here for anyone watching. We're closing this in favor of #9709 and plan on taking a different approach. We are not going to export usePrompt, but rather rely on useBlocker being the low-level hook needed to implement prompt behavior in userland.

@chaance chaance closed this Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants