remix-run / history

Manage session history with JavaScript
MIT License
8.29k stars 961 forks source link

Difficult to migrate .block() use case to v5 #921

Open liamqma opened 2 years ago

liamqma commented 2 years ago

Hi,

I use history.block to check whether the next location can be transition to. If not, do a full page load. The pseudo code looks like

const unblock = history.block((tx) => {
  // canTransition is the function that checks if next location can be SPA transition to
  // If false, do a full page load on next location's path
  if (!canTransition()) {
    return window.location.assign(nextPath);
  }
  unblock();
  tx.retry();
});

Full code example: https://codesandbox.io/s/distracted-river-8yczz?file=/src/index.js

So depending on the result of canTransition(), it should either unblock the blocker or do a full page reload. Users shouldn't notice much difference. This is fine except for the navigation that reloads the page. Users see something like this (Chrome): image It doesn't make sense to our users as there isn't any unsaved changes. I've read the documentation and understand the library registers a beforeunload handler to prevent the navigation. Maybe v5's .block is not designed for my use case.

But I am able to achieve this use case via history v4. I've also tried history.listen but .listen is too late as it happens at the end of applyTx.

Any suggestion is much appreciated. 🙂 The solution I can think of is to remove window.addEventListener(BeforeUnloadEventType, promptBeforeUnload); from the library and let consumer of the library to handle it.

liamqma commented 2 years ago

I've found a hacky workaround 😬

window.addEventListener('beforeunload', () => {
        unBlock();
});

Register our own BeforeUnloadEvent and clean up our transition blockers before history runs its check

dubzzz commented 2 years ago

@liamqma Thank you so much for the workaround it works perfectly well in my case too. I just had to register my own event listener before calling block which I did the other way initially.

Meanwhile I am wondering if calling the provided blockers on "beforeunload" could be something directly implemented inside history itself. I came up with this code on my side (highly inspired from react-router beta.6):

// location being undefined when beforeunload cases
export type BlockerFunction = (tx: { location: Location | undefined; retry: () => void }) => void;

export function useBlocker(blocker: BlockerFunction): void {
  const navigationContext = useContext(UNSAFE_NavigationContext);
  const navigator = navigationContext.navigator as History;

  useEffect(() => {
    const beforeUnloadCallback = () => {
      let closeAuthorized = false;
      blocker({
        location: undefined,
        retry: () => {
          closeAuthorized = true;
        },
      });
      if (closeAuthorized) {
        unblock();
      }
    };
    window.addEventListener('beforeunload', beforeUnloadCallback);

    const unblock = navigator.block((tx) => {
      const autoUnblockingTx = {
        ...tx,
        retry() {
          unblock();
          tx.retry();
        },
      };
      blocker(autoUnblockingTx);
    });

    return () => {
      unblock();
      window.removeEventListener('beforeunload', beforeUnloadCallback);
    };
  }, [block, blocker]);
}

The beforeUnloadCallback is basically doing the trick you talked about plus calling the underlying blocker to check whether or not we should prevent the default behaviour of the browser. If we should, then we let the beforeunload handler registered by history do the blocking, otherwise we unplug it (your code).

I'm wondering if such behaviour should be the default for block in history. It would made users able to say whether or not they really want to lock the tab closing. Is there a reason not to call blockers when receiving things on beforeunload?

liamqma commented 2 years ago

@dubzzz this approach sounds like a legit improvement.

karlshea commented 2 years ago

I'm trying to migrate to this version, and we were using a similar approach (checking to see if a route should be handled by react-router in our SPA or if we needed to fully reload the page to get a server-rendered version), and blockers are completely not working for us to the point where we may have to fork this library.

The big issue is the beforeUnload event listener that history is registering. It keeps popping a prompt when reloading the page or when navigating in and out of our SPA pages using the back/forward browser buttons, even when we register our own listener and unblock() first.

I think what we really need is a persistent listener that we don't need to keep registering on every route change that can check the same transaction but doesn't register the beforeUnload listener.

This will all be moot when we finish moving the whole app to a SPA, but for anyone that has some routes in a SPA and some routes outside, you really cannot move to history 5 or react-router 6 at this point.

karlshea commented 2 years ago

I was able to get this working in my case by making the changes in PR #949.

Then in our app I set up the blocker like this:

const registerBlock = () => {
  const unblock = history.noUnloadBlock((tx) => {
    if (!isSpaLocation(tx.location)) {
      window.location.href = createPath(tx.location);
      return;
    }

    unblock();
    tx.retry();
  });
};

history.listen(() => {
  // Register a new block on location changes.
  registerBlock();
});

// Register initial block.
registerBlock();

Normal blockers are looked at first (to support prompts), and then if they all allow the transaction the no-unload blockers are checked afterwards.