React strict mode bug

[catamphetamine]

A scroll-behavior instance is created in the constructor() of the <ScrollManager/> component.
According to React strict mode docs, that's an anti-pattern that results in incorrect operation.

• Component instance A is created
• Component instance B is created
• componentDidMount() is called with this being B
• componentWillUnmount() is called with this being B
• componentDidMount() is called with this being B

Because of that, clean-up of Component instance A is not run and a scroll event listener with stale props (initial location) responds to scroll events forever.

A fix would be creating a scroll-behavior instance in componentDidMount().

I also rewrote <ScrollManager/> in React hooks in case you're interested (it also contains the fix for the issue):
https://github.com/catamphetamine/react-pages/blob/186eedf4ab427d512e9332926d3d488b307f311f/lib/router/client/found-scroll/ScrollManager.js

I also published a rewrite of farce + scroll-behavior as a new package:
https://www.npmjs.com/package/navigation-stack

Here's the source code for the <ScrollManager/> from the link above copy-pasted:

import { StateStorage } from 'farce';
import React, { useRef, useEffect, useMemo, useCallback, createContext } from 'react';
import ScrollBehavior from 'scroll-behavior';

const STORAGE_NAMESPACE = '@@scroll';

export const ScrollContext = createContext(null);

const defaultCreateScrollBehavior = config => new ScrollBehavior(config);

// Rewrote `<ScrollManager/>` from `found-scroll` in React hooks.
// Also fixed React strict mode bug.
// https://github.com/4Catalyzer/found-scroll/issues/382
export default function ScrollManager({
  renderArgs,
  shouldUpdateScroll: shouldUpdateScrollProperty,
  createScrollBehavior = defaultCreateScrollBehavior,
  children
}) {
  const { router, location } = renderArgs;

  const prevRenderArgs = useRef(null);
  const scrollBehaviorRef = useRef();

  const scrollBehavior = scrollBehaviorRef.current;

  const getCurrentLocation = useCallback(() => {
    return location;
  }, [location]);

  const getCurrentLocationRef = useRef();
  getCurrentLocationRef.current = getCurrentLocation;

  const shouldUpdateScroll = useCallback((prevRenderArgs, renderArgs) => {
    if (!shouldUpdateScrollProperty) {
      return true;
    }
    // A hack to allow access to `ScrollBehavior` internals (e.g. `stateStorage`).
    return shouldUpdateScrollProperty.call(scrollBehavior, prevRenderArgs, renderArgs);
  }, [scrollBehavior]);

  const registerScrollElement = useCallback((key, element) => {
    scrollBehavior.registerElement(key, element, shouldUpdateScroll, renderArgs);
    return () => {
      scrollBehavior.unregisterElement(key);
    };
  }, [
    shouldUpdateScroll,
    scrollBehavior,
    renderArgs
  ]);

  const scrollContext = useMemo(() => ({
    scrollBehavior,
    registerScrollElement
  }), [
    scrollBehavior,
    registerScrollElement
  ]);

  useEffect(() => {
    const scrollBehavior = createScrollBehavior({
      addNavigationListener: router.addNavigationListener,
      stateStorage: new StateStorage(router, STORAGE_NAMESPACE),
      getCurrentLocation: () => getCurrentLocationRef.current(),
      shouldUpdateScroll: (prevRenderArgs, renderArgs) => shouldUpdateScroll(prevRenderArgs, renderArgs)
    });
    scrollBehaviorRef.current = scrollBehavior;
    return () => {
      scrollBehavior.stop();
    }
  }, []);

  useEffect(() => {
    const scrollBehavior = scrollBehaviorRef.current;
    const prevLocation = prevRenderArgs.current && prevRenderArgs.current.location;

    if (renderArgs.location === prevLocation || !(renderArgs.elements || renderArgs.error)) {
      // If the location hasn't actually changed, or if we're in a global
      // pending state, don't update the scroll position.
      return;
    }

    scrollBehavior.updateScroll(prevRenderArgs.current, renderArgs);
    prevRenderArgs.current = renderArgs;
  });

  return React.createElement(ScrollContext.Provider, {
    value: scrollContext
  }, children);
}

[rosskevin]

@catamphetamine is there a reason you haven't PR'd your hooks fix to this repo?

[catamphetamine]

@rosskevin I guess the reason is that I completely rewrote the component and that would be not a small PR as a result. More like complete file replacement rather than a small diff of it. So PR or not PR, it would be easier to just copy-and-paste the file by replacing the old one in this library. PRs are mostly for smaller diffs.

[rosskevin]

Perhaps you misunderstand. PRs are for improvements. This is an improvement and I for one would like to use it. I do not want to copy/paste improvements of code from the ecosystem into my source tree, I want to share the burden of maintenance/improvements with the rest of the world via the original repos/dependencies.

I looked where you put the code and it seems like you are trying to copy/paste all kinds of dependencies into your repo but not give back to the sources. I don't think you are heading down a path that is sustainable.

[catamphetamine]

Perhaps you misunderstand.

Nah, I don't.

PRs are for improvements.

Yes.

This is an improvement and I for one would like to use it.

And you can do so by copy-pasting the file to your project.

I want to share the burden

Your choice. I'll pass.

I looked where you put the code and it seems like you are trying to copy/paste all kinds of dependencies into your repo but not give back to the sources.

I wonder how making the source codes publicly available on the Internet leads to "not give back to the sources".

I don't think you are heading down a path that is sustainable.

So fixing issues of an open-source repo and then showing the fix to the maintainers is somehow non-sustainable to a casual passer on the Internet. Ok.

[catamphetamine]

@rosskevin I dunno if GitHub still notifies a user when someone from the blacklist messages them. In any case, I've blocked you due to the apparent non-constructive/aggressive behavior in your comments.