Does it support react-router v4 ?

[suhaotian]

No description provided.

[taion]

No. RRv4 has no concept of router middlewares.

[ntucker]

Are there plans for something like this to be built for v4?

[taion]

I do not plan to do so, no.

[mijamo]

I have forked this repo to support React Router v4. The result is here : https://github.com/ytase/react-router-scroll

Minor adjustements were required (like saving the data in sessionStorage instead of the state of history).

I added some doc in the Readme and warning. The library seems to be working as expected, but I haven't taken time to fix the tests yet.

Anybody is obviously free to use my fork to have that great library working under React Router 4, and help bringing something more solid into life (fixing tests, maybe changing some other things in the code etc.).

I can also create a pull request if you are interested, though it is still pretty early for that.

[ntucker]

@mijamo I'm curious why you needed to change the storage backend.

[mijamo]

As you can see here https://github.com/taion/react-router-scroll/blob/master/src/StateStorage.js the storage backend was using DOMStateStorage from the historypackage, which was using sessionStorage under the hood (https://github.com/ReactTraining/history/blob/v3/modules/DOMStateStorage.js for reference).

React Router v4 uses the new version of history that does not have that module anymore. As I needed some very quick fix and didn't have much time I just used sessionStorage straight away. Another possibility would have been to copy that file from the old version of history into the repo, but my opinion was that handling storage of information is probably a separate concern and thus should be handled by something external. The current storage backend is merely a temporary fix that works in most of the cases, and should be replaced by something proper from another library instead of just bringing in more complexity.

Maybe a next step would be to use something like https://github.com/nbubna/store which is widely used for that kind of problem.

[taion]

Isn't that implementation going to do weird things when there are redirects?

[mijamo]

I have not tried yet, I do not use redirects with react router. As I said it is very early, I just wanted to share my early experiment in case anybody is interested. For now it is working good when I test it but there are tons of improvements possibles and that would be needed to have something ready to be published and widely used.

What is your intuitive thought regarding redirects and in which situation would they cause trouble?

[taion]

RRv4 re-renders the page on a redirect, right? so it will try to scroll the page. also render a blank page (ish) and mess up any saved scroll position

[mijamo]

I tried quickly with a Redirect and did not encounter the problem you are suggesting. When finding a Redirect, react router just does a history.replace.

So let's say you are at /with key abc, you click a link to /page1, the new location has pathname /page1and key def and you are redirected to /page2 so the new location has pathname /page2 with key ghi but the history is actually abc -> ghi so if you hit previous you will go directly to abcwithout passing by def so it will restore the scroll from abc without any interference from the intermediate redirection. Same if you then hit next, it will just restore ghi.

Or have I misunderstood what you meant?

[taion]

The issue I'm thinking of is that, if you click on a link that redirects you, the scroll update will fire twice – once on the page that renders the redirect, then once on the page you actually want to get to. It may well not leave anything broken, but I think it could look janky, especially if the scroll destination is a hash anchor or partway down the page or something.

[faceyspacey]

Unfortunately history.listen for theaddTransitionHook has a major problem. It fails to meet this criterion:

The transition hook function should be called immediately before a transition updates the page

The location.key used to save in sessionStorage is the wrong key. It's the key of the following page since the new state has already been pushed on to the globalHistory object

I.e. this:

https://github.com/ReactTraining/history/blob/master/modules/createBrowserHistory.js#L172

is called before this:

https://github.com/ReactTraining/history/blob/master/modules/createBrowserHistory.js#L183
which notifies listeners.

It's the same for replaceState, pop events, etc.

So that means when you return to the page and the<ScrollContainer /> component mounts, they will attempt to reload their scroll position using the correct location.key and it won't work.

To be clear, I'm talking about the lesser used component for when you want scrollable elements, not the window.

However, the patch here (affecting window) may have broken as well:
https://github.com/taion/scroll-behavior/blob/master/src/index.js#L48

But I've yet to have that issue so I can't test it.

ps. It's the reason the original react-router-scroll used router.listenBefore, whish isn't available in the plain history package.

[faceyspacey]

A hacky solution I've been playing with is keeping a record of the previous location.key, and using that for <ScrollContainer /> saves instead.

[catamphetamine]

@faceyspacey You seem to know react-router-scroll internals.
I myself am not migrating to react-router@4 yet but in some future that's likely to happen (say, in 2018).
So if you happen to have any thoughts which can benefit future seekers make sure you leave them here.
I personally didn't understand anything from your comments because they're intended for someone familiar with the inner workings of react-router-scroll.

[KyleAMathews]

@mijamo working on upgrading Gatsby to use RRv4 and dropped your fork in and after some basic testing seems to be working great! Will be testing more and contributing back if needs be. Perhaps we should turn your fork into a real project? Or @taion would you be interested in letting us use this repo for the v4 version of this module?

[faceyspacey]

@halt-hammerzeit will do.

@KyleAMathews basically, the issues mentioned here should probably be fixed before it becomes its own repo. I actually made a repo for a reworking of this for redux-first-router:

https://github.com/faceyspacey/redux-first-router-restore-scroll

I fixed 2 of the 3 issues, but can't replicate the one i mentioned above. ..and ultimately 1 of the issues doesn't apply to redux-first-router (redirects on rendering). So I only fixed one.

The hard part really is React Router will need to expose a listenBefore hook of its own. It won't be able to come from the history package. So you should ask the maintainers at React Router (or provide a PR) for this. Bring it up in the forum over there, they'll know what you're talking about because the old router did have this method.

[taion]

I'd be okay with merging the RRv4 bits, assuming the outstanding issues are resolved.

[esprehn]

What's that status of this? Should folks move to the fork?

[taion]

If it's working for everyone, we can merge it, I guess. I'd rather the issues here be addressed, but probably if it works in practice, the theoretical issues are not so big a deal.

[holloway]

Here's a draft of an approach that I'm using. I haven't read the react-router-scroll code but maybe this technique can be used?

To use it just import and call scrollMemory().

import { browserHistory } from 'react-router';

export const scrollMemory = () => {
  const ATTEMPT_RESTORE_MILLISECONDS = 500;
  const REATTEMPTS_TO_SCROLL = 10;

  const memory = {};
  let wasUserGeneratedScroll = true;

  const getScrollTop = () => {
    if (document.body && document.body.scrollTop) {
      return document.body.scrollTop;
    } else if (document.documentElement && document.documentElement.scrollTop) {
      return document.documentElement.scrollTop;
    }
    return 0;
  };

  const stringifyLocation = (location) => {
    return `/${location.pathname.replace(/^\//, '')}${location.search
      ? `?${location.search.replace(/^\?/, '')}`
      : ''}${location.hash ? `#${location.hash.replace(/^#/, '')}` : ''}`;
  };

  window.addEventListener('scroll', () => {
    const location = browserHistory.getCurrentLocation();
    const url = stringifyLocation(location);
    memory[url] = getScrollTop();

    // if they're currently in the middle of attempting
    // to restore a scroll position...
    if (restoreScrollTimer && wasUserGeneratedScroll) {
      // the user has scroll themselves during attempting
      // to restore scroll positions so cancel further attempts
      clearTimeout(restoreScrollTimer);
    }
  });

  let restoreScrollTimer;
  const eventuallyScrollTo = (scrollTop) => {
    if (restoreScrollTimer) {
      clearTimeout(restoreScrollTimer);
    }
    let remainingAttempts = REATTEMPTS_TO_SCROLL;
    const attemptScroll = () => {
      remainingAttempts--;
      wasUserGeneratedScroll = false;
      window.scrollTo(0, scrollTop);
      requestAnimationFrame(() => {
        wasUserGeneratedScroll = true;
      });
      if (remainingAttempts > 0 && getScrollTop() !== scrollTop) {
        restoreScrollTimer = setTimeout(
          attemptScroll,
          ATTEMPT_RESTORE_MILLISECONDS
        );
      }
    };
    attemptScroll();
  };

  browserHistory.listen(location => {
    // fired when the browser changes urls
    const url = stringifyLocation(location);
    if (memory[url]) {
      // there's been a url change and we have a new scroll
      // position to restore
      eventuallyScrollTo(memory[url]);
    }
  });
};

[davidfurlong]

I'm sharing my current solution, as I think it's quite nice and I haven't seen it elsewhere in this form (using react-router location state).

It defaults to scrolling to the top on location change (and lets the browser handle "Back" and "Forward" scroll state -- which I've tested to work well in Firefox & Chrome newest versions). It also allows for you to change location without resetting on a case by case basis, using react-router's location state.

// ScrollToTop.js
import React from 'react';
import { withRouter } from 'react-router-dom';

class ScrollToTop extends React.Component {
  componentDidUpdate(prevProps) {
    if (prevProps.location && this.props.location !== prevProps.location &&
      !(this.props.location.state && this.props.location.state.resetScroll === false)) {
      window.scrollTo(0, 0);
    }
  }

  render() {
    return this.props.children;
  }
}

export default withRouter(ScrollToTop);

// client.js
...
<ScrollToTop>
 <App />
</ScrollToTop>
...

// Example usage:
history.push('/about'); // Reset the scroll
history.push('/tab/1', { resetScroll: false }); // Don't reset the scroll

[taion]

For the time being, why not just use @ytase's react-router-scroll-4? I can't guarantee correctness, but it's probably closer to what you want.

[ntucker]

@ytase Can you update to use new context instead of legacy?