Separate history object to its own context

[illuminist]

Reference #6999

As a suggestion from @MeiKatz . I added a new context for history object only to be used for useHistory hook and prevent the hooks from getting update triggerred by the original context.

The original context still has all the same properties without any change, the history object is also in the original context.

Tested with my project and it reduced rerendering greatly.

[timdorr]

Besides useHistory, are there other places we can use the HistoryContext instead of RouterContext?

[illuminist]

It's so embarrassing I missed that.

[illuminist]

Besides useHistory, are there other places we can use the HistoryContext instead of RouterContext?

Other components are still relying on other properties from the main context. I think I can separate other context properties as well, so there will be separate context for each properties: history, location, match, and staticContext. Should I do that now or wait until this branch get merged?

Edit: Forgot to check react-router-dom, found some potential candidates.

[esealcode]

Hi,

Sorry for not being much present in the process of my issue the last weeks.

@illuminist about separating each props into their own contexts i'm not sure of the benefit it would give since the original issue was about history prop still getting dispatched along other updated props (while remaining unchanged itself) on a route change. However when a route change occurs it's very likely that location and match props will both be updated in the process meaning separating them into seperate contexts probably won't add any performance improvement.

As for the staticContext i'm not sure why it should be touched at all as it seems to be related to SSR (not using SSR here sorry).

Since the original issue was just about the useHistory performance concern and the pull request actually addresses the issue it should be a go for the merge and moving the separation of the others props to a new issue if needed.

Great work :)

[djamaile]

When will this be merged?

[jamesthomsondev]

Also wondering when this will get merged. We're experiencing a lot of component re-renders due to useHistory.

[illuminist]

@timdorr There isn't much need to change right now from last time I committed. Do you think is it good to merge?

[AlithAnar]

+1 for merge

[timdorr]

Yeah, let's do it. I'll see if I can get this out soon.

[Kutalia]

What if I want to force component rerendering on history changes while maintaining useHistory?
Let's say I have navigation buttons in my router component. Using the history object I can deduce whether I am on the first page (history.index === 0) or on the last page (history.index === history.length - 1) and show or hides buttons accordingly. But I need to rerender the component on each history.push.
I've found a hack for it - useLocation. It acts like a subscriber for history updates. Is there a cleaner way to make it work?

[esealcode]

@Kutalia The history object never changes (it's a mutable object). This is what caused my initial issue which led to this PR because the history object was shipped along some other props (like location) in the same context together causing unnecessary updates from the useHistory when location changed.

Hence if you were relying on useHistory to listen for location updates you were doing it wrong even if it unintendedly worked. You want to use history.listen (see: https://github.com/ReactTraining/history/blob/2e9e5118a33e5b62ac3eb74f2fbc80d7e7a30dde/docs/GettingStarted.md#basic-usage) to listen for every location changes and read history props from there to determine if you need to update your component or not.

[Kutalia]

I need to track history updates (more specifically, history.index and history.length, I am using a MemoryRouter), and rerender a component. I found a workaround:

const history = useHistory();
const [, forceUpdate ] = useState();

useEffect(() => {
    return history.listen(() => {
        forceUpdate({});
    });
}, [history]);

But I am afraid forcing component rerendering in a such way is not a recommended pattern.

[illuminist]

@Kutalia Another workaround is just putting useLocation into the same component as it will trigger component update when location change which will also depend on history state.

But the best solution should be creating a custom hook that provide the information about history object.

const useHistoryState = () => {
  const history = useHistory()
  const [index, setIndex] = useState(history.index)
  const [length, setLength] = useState(history.length)
  useEffect(()=> {
    return history.listen(() => {
      setIndex(history.index)
      setLength(history.length)
    })
  }, [history])
  return { index, length }
}

You can add more property from history object as you like.

[Kutalia]

Thanks for the suggestion. As I actually use history.goBack and history.goForward as well I think I'd better stick with the "forceUpdate" method instead of returning them separately from the custom hook. This way I'll avoid over-abstraction.