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

Remove history.block #690

Closed
mjackson opened this issue Mar 27, 2019 · 22 comments
Closed

Remove history.block #690

mjackson opened this issue Mar 27, 2019 · 22 comments

Comments

@mjackson
Copy link
Member

mjackson commented Mar 27, 2019

I'd like to remove history.block. This is a major, breaking change (we're removing the functionality altogether), and I'd like to explain our rationale.

The use case

history.block was originally introduced to support React Router's <Prompt> component. The purpose of a <Prompt> is to prevent a user from navigating away from a page when they have entered some data that hasn't yet been saved, e.g. a half-filled-out form. history.block provides a way for a <Prompt> to prevent the location from changing (and thus prevent the router from showing a new page).

All other uses of history.block are abusing this API for some reason other than for what it was intended.

The problem

The main reason I'd like to remove this API is because it is incomplete, and we can never make it complete, so we need to find another way to satisfy the use case. If you've ever browsed the source, you have have noticed some significant TODOs around the code that tries to restore the last location when navigation is denied.

This is because there is no way we can actually prevent the user from changing the URL, so we have to try and fake it. How?

  • When the URL changes, fire all the history.block callbacks
  • If any one of them prevents navigation (e.g. returns false), try to revert the URL change

Now, we say try to revert the URL change because the simple fact is there is no API in the browser that tells you the last URL you were at. This is a security measure, obviously. Browsers don't want to let you run code that tells you where users came from (with the exception of document.referrer, but I digress).

But I tried to hack it (I know, I'm a terrible person. This is my penance).

What I did was I keep an array of all locations that the user has visited in memory. Then, when navigation is cancelled I use the index of the last location's key to try and figure out how many entries we need to go in order to restore the last URL. Needless to say, this has a number of drawbacks, not the least of which is that it won't work after a page refresh. So this is a dead end.

I originally shipped it thinking it'd be good enough for most apps, and it has worked ok for some apps. But it has also created a lot of headache for people who have run into the edge cases.

The solution

It's not difficult to figure out your own strategy for a good user experience in this situation. It will probably boil down to something like this:

  • As the user is filling out the form, save the form's state to localStorage (or sessionStorage)
  • When they submit the form, clear the temporary state from localStorage so you know it has been submitted successfully
  • When they navigate away and get to the new page on your site, if there is some data they haven't submitted either:
    • ask them if they'd like to return to the form and submit it or
    • do nothing and pre-populate the form if they ever come back.
  • If they navigate to a new site, either:
    • use the beforeunload event to prompt them to stay and submit the form first or
    • do nothing and pre-populate the form if they ever return or
    • clear your local cache (don't pre-populate it when they return)

It's not hard to imagine wanting to handle some unsaved state in several different ways.

The upsides

  • You'll have more flexibility to provide a custom UX that's best for your site
  • Your site will load faster because we'll get to shed quite a bit of code in this library that'll make your whole page lighter
  • Your site will have fewer bugs because you're not relying on my hacks

The downsides

  • You'll have to do some thinking and rework any areas where you're using history.block because I can't solve this problem for you in a reliable way

I realize this is going to make some people upset (breaking changes always do) but I've tried my best to be open and explain the rationale behind this decision while still providing a way forward. Happy to hear your feedback and questions if you have them.

@mjackson
Copy link
Member Author

There is also an issue with history.block on iOS that we cannot solve, see #673

@lstkz
Copy link

lstkz commented Mar 27, 2019

Vue and Angular can provide "block navigation" functionality, so it's doable.

This project has 3M weekly downloads, and there are no alternatives for React. I am not able to implement your suggested solution in my current project because it will add too much complexity, and will require rewriting thousands of lines of code. Probably, the client won't like localStorage solution either.
I will have to use an old version of history and have technical debt or fork it and maintain it by myself.

I really like React, but the whole ecosystem is painful.
It seems like my answer to "Unpopular opinion: @reactjs edition" by @gaearon is true.

@gabrielmontagne
Copy link

gabrielmontagne commented Mar 27, 2019

Hi, we are happily abusing history.block in our project, . It has, in a way that reads nicely, even though it was not the original purpose, allowed us to review and control location in our state and decide that, for certain URLs, we want to perform certain actions (preloading, etc. ) before the app actually "goes there" . We're using RxJS and this fits very nicely into a push flow. This is too short a summary, but components don't have to cater for missing data, etc. because we have a very clean way to prevent certain URLs from being rendered until all is ready.

CC @cristiano-belloni

@gabrielmontagne
Copy link

... we are even wrapping our little library around it : / https://github.com/zambezi/caballo-vivo which we use internally on a few very large projects in banking.

@cristiano-belloni
Copy link

cristiano-belloni commented Mar 27, 2019

@mjackson - I didn't see the code (yet), but I was thinking - if you remove it, would it be possible to have a way to hook into history and re-implement your block "hack" on our side? That would still give history the upsides you mention, while it would mitigate the downsides on our side.

@mjackson
Copy link
Member Author

mjackson commented Mar 27, 2019

@BetterCallSKY I know for a fact that both the Vue and Angular routers are heavily based on React Router (Vue's history lib in particular is almost a direct copy of this library), so I would not be surprised if their implementations of this feature are subject to the same limitations we are. Instead of just saying "Vue and Angular can do it" and lamenting, you should actually go and build a feature of your app in either of those libraries, post it here, and I'll show you how it has the same limitations I've described above.

@cristiano-belloni Yes, it shouldn't be too hard to build out this functionality on your own. Something like this could work:

history.listen(location => {
  if (!window.confirm('Are you sure you want to leave?')) {
    // This is the hard part. You can naively say "go back", but you can't be 100% sure
    // that will take them to the location they came from. It's just a guess.
    history.goBack();
  }
});

@lstkz
Copy link

lstkz commented Mar 27, 2019

(Vue's history lib in particular is almost a direct copy of this library)

https://github.com/vuejs/vue-router/tree/dev/src/history
For me, it's a completely different implementation.

https://codesandbox.io/s/jzr5nojn39
Example confirm navigation in Vue. It seems to work correctly.
Most people expect to have a similar feature in React.

@mjackson
Copy link
Member Author

https://github.com/vuejs/vue-router/tree/dev/src/history
For me, it's a completely different implementation.

@BetterCallSKY Look closer, my friend. There are numerous instances where the code in that library is copied straight from v3. One example: here's us and here's them ... even the comments were copied from us 😅 It's been a few years, so the code has changed over time. But when that library started it was a pretty direct copy of this one.

https://codesandbox.io/s/jzr5nojn39
Example confirm navigation in Vue. It seems to work correctly.

I made a little video to show how that example is broken:

vue-router

Here are the steps to reproduce:

  • Start on the home page
    • The history stack is empty
  • Click through pages 1 and 2, confirming each step
    • Now you're on page 2
    • The history stack has 2 entries in it: page 1 and the home page
  • Using a click and hold on the back button, navigate back to the home page, but cancel this navigation
    • You're still on page 2 (the navigation was blocked)
    • But the history stack only has 1 entry now!
  • Click the back button
    • You should be on page 1, since that was the page you visited before page 2
    • But instead, you're on the home page

This is because when Vue's router cancelled the navigation from page 2 back to the home page, it could not tell where the user was coming from, so it did the naive thing and pushed page 2 back onto the stack. That's the limitation that I was trying to describe in the OP.

@lstkz
Copy link

lstkz commented Mar 27, 2019

Thanks for explaining.
Not sure what other people think about it, but for me, it's a very edge case I can live with it.
It's better to lose a few history entries than the whole form 😉

@ryanflorence
Copy link
Member

@BetterCallSKY It's easier, safer, and less code to save the form into sessionStorage.

@mjackson
Copy link
Member Author

Not sure what other people think about it, but for me, it's a very edge case I can live with it.

@BetterCallSKY I'm not saying we can't have a naive <Prompt> component in React Router that just does history.goBack() when navigation is cancelled. We can. We just don't need to support it at this level. The history.block API is the wrong place for it because it is misleading.

@mjackson
Copy link
Member Author

mjackson commented Mar 28, 2019

@cristiano-belloni I was thinking a bit more about your question, and I think this is probably the best generic solution I can come up with:

// Use this variable to keep track of the most recent location.
let prevLocation = null;

// Be sure to call this on the initial page load so that you get
// the initial POP.
history.listen((location, action) => {
  if (prevLocation) {
    if (!window.confirm('Are you sure you want to leave?')) {
      // They want to stay. Try to revert the last location change.
      if (action === 'PUSH') {
        // If it was a PUSH, goBack() should get us back to the last URL we were at.
        history.goBack();
      } else if (action === 'REPLACE') {
        // If it was a REPLACE, we can probably just do another replace with
        // the prevLocation.
        history.replace(prevLocation);
      } else { // action = POP
        // This is the hard part. We can only guess what to do here because we
        // cannot know where they came from. Should we goBack()? Should we
        // push(prevLocation) like Vue does??
      }

      return;
    }
  }

  // Save this reference for next time.
  prevLocation = location;
});

@cristiano-belloni
Copy link

Thanks, @mjackson. Will try this as soon as possible!

@lstkz
Copy link

lstkz commented Mar 28, 2019

It's easier, safer, and less code to save the form into sessionStorage.

Applications can have various complexity.
For example: if you open multiple deep modals/panels it's much better to show 'unsaved changes modal' instead of trying to save everything in sessionStorage and restore it. Not all applications have only simple forms...

It's definitely not right that you create a low-level library and you force people on high-level UX.

@mjackson
Copy link
Member Author

It's definitely not right that you create a low-level library and you force people on high-level UX.

@BetterCallSKY Please stop. I would like to limit discussion here to helping people instead of arguing about what's "right" and what's not.

@TheFrogPrince
Copy link

So... I just wanted to hop in here and say: I hate to see folks give up like this. history.block is working perfectly for us, even on iOS. But I do understand being beaten up by edge cases until one finally just gives up.

I do have a question, though.

My initial reaction is to just wire my own hook into the transition manager to replace the method you're about to rip out... but it suddenly becomes unclear to me when you started boasting of significantly smaller code, is your intention to rip out the entire transition manager?

I did try your history.listen suggestion above in our project, but it breaks because we are using (not my choice) a yucky where we

history.push("/nullRoute"); if (history.location.pathname === "/nullRoute") { history.replace(this.props.to); }

in order to force a component refresh (it's an IFrame component). Certainly not trying to defend this decision here, but when history.listen fires on the .push, the code calls the .back method, but then the location.pathname isn't actually switched back before that if condition occurs on the next line, so according to the code, it looks like the user approved the switch even though they didn't.

Without an explicit history.block method, I really do need to hook into the transition manager directly so I can set things up myself, and then things would update inline, which is what we need.

So I guess I'm hoping the plan is keep it (the transition manager)? :)

@TheFrogPrince
Copy link

TheFrogPrince commented Apr 15, 2019

So the way I'm reading it, it looks like you would need to expose the transitionManager here before people could start implementing their own replacement for block:

const history = {
length: globalHistory.length,
action: 'POP',
location: initialLocation,
createHref,
push,
replace,
go,
goBack,
goForward,
block,
listen
};

return history;

@l0gicgate
Copy link

It’s absolutely doable. I’ve actually forked the lib and implemented it myself the right way. First of all, you need to add functionality that enables you to toggle dispatching events to the listeners on/off.

Once you’re in control of that, you can simply replace the url to the previous URL when a user tries to navigate away without notifying the listeners as if nothing has changed.

danielnixon added a commit to oaf-project/oaf-react-router that referenced this issue May 17, 2019
danielnixon added a commit to oaf-project/oaf-navi that referenced this issue May 17, 2019
@jtomaszewski
Copy link

jtomaszewski commented Jun 26, 2019

What if we would keep the allKeys array in the history API state object?

Then for a given history state, we would know:

  • what were the previous browser paths before it
  • or nothing (in case that this JSON data is lost i.e. on browser restart).

This would allow me to have a canGo functionality in the browsers. ( remix-run/react-router#1066 ) There's one library that kind of does it but it does it imperfectly ( I described it here hinok/react-router-last-location#24 ).

Maybe that would also make the history.block functionality possible?

@micaste
Copy link

micaste commented Aug 6, 2019

@mjackson I also tried the suggestion you proposed in #690 (comment), but as @TheFrogPrince said it wasn't equivalent:

  1. The history.goBack() was actually making the app render the previous path and then "going back" to the current. On top of the blinking, the issue was that it was clearing the local state of my form by forcing an unmount/mount of my form. It does not blink when using .replace only.

  2. It seems that the .listen method does not catch manual changes in the URL, or the use of the "previous" button in the browser, whereas the .block method does.

Like @TheFrogPrince , I would be okay with the removal as long as there is a way to build the feature with a public API.

@mjackson
Copy link
Member Author

mjackson commented Sep 6, 2019

@TheFrogPrince The "transition manager" is not even public API. It's purely an implementation detail. Whether it stays or goes is irrelevant.

you can simply replace the url to the previous URL

@l0gicgate If you do this your app will be broken in the same way I demonstrated in #690 (comment)

After thinking about this a bit more I think there may be a way we can keep history.block but with a different API. I'm going to close this. Current plan is to ship an improved version in v5.

@mjackson mjackson closed this as completed Sep 6, 2019
@micaste
Copy link

micaste commented Sep 7, 2019

Thanks for the follow-up.

Current plan is to ship an improved version in v5.

Looking forward to seeing it !

@lock lock bot locked as resolved and limited conversation to collaborators Nov 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants