[Bug]: MemoryHistory does not emit events using listen

[christian98]

What version of React Router are you using?

v6.4.0

Steps to Reproduce

import { createMemoryHistory } from '@remix-run/router';

it('listen new', () => {
    const memoryHistory = createMemoryHistory();
    const callback = jest.fn();

    memoryHistory.listen(callback);
    expect(callback).not.toHaveBeenCalled();
    memoryHistory.push('/new');
    expect(callback).toHaveBeenCalled();
});

Expected Behavior

I would expect that the callback I pass to the listen method gets called when I push a new route. Also when using the unstable_HistoryRouter I would expect, that the router actually changes the content.

Actual Behavior

The test fails because the callback does not get called. When I look into this repo I can spot these sections which might be wrong, or copied wrong.

Old listen in history: https://github.com/remix-run/history/blob/3e9dab413f4eda8d6bce565388c5ddb7aeff9f7e/packages/history/index.ts#L993

New listen in @remix/router:

react-router/packages/router/history.ts

Line 287 in cb7f510

listener = fn;

I think this test is misleading here, no?

react-router/packages/router/__tests__/TestSequences/ListenPopOnly.ts

Line 12 in cb7f510

expect(spy).not.toHaveBeenCalled();

Actually, this is also why for me the unstable_HistoryRouter does not work. I am currently using it for some tests.

[timdorr]

It's because the default v5Compat option is false:

react-router/packages/router/history.ts

Line 208 in cb7f510

let { initialEntries = ["/"], initialIndex, v5Compat = false } = options;

That is failing this check before calling the listener:

react-router/packages/router/history.ts

Lines 267 to 269 in cb7f510

	if (v5Compat && listener) {
	listener({ action, location: nextLocation });
	}

@brophdawg11 Do you know why we don't run listeners by default in @remix-run/router?

[brophdawg11]

Yep! In react-router@6.3 (using history@5), there was no "router" per-se, just a navigator that triggered history, and react-router simply rendered in response to all history updates (push/replace/pop). So a link click went something like:

click -> navigator.push -> history.push -> history.listen -> render

With the introduction of the data APIs and router/RouterProvider, we now fetch data before updating history. This more closely mimic's browser behavior, and also prevents weird UI issues (like flickering URLs during redirects and such). This means the state is lifted upwards to the router, and we don't need to listen to push/replace history updates because the router has already been updated by the time we touch history so we render via it's own subscriber and updating history happens sort of as a side effect.

navigator.push -> router.push -> fetch data -> router.subscribe -> render
                                            \
                                             history.push
                                             

POP is the exception here, since we can't intercept a back button click prior to history being updated, so we're still in a listener mode there.

v5Compat enables listening for push/replace and it used by the non-data routers (BrowserRouter, etc.)

history was inlined in 6.4 instead of a new history@6 package with this updated behavior because the router is intended to be the primary point of interaction, and history is more of an implementation detail. This should also help future proof for the Navigation API when it stabilizes.

[christian98]

Alright, for me it is currently enough that I can specify the v5Compat option (Did not see that in the code :D ). So, I'll close this, as this does not seem to be a bug, but rather an intended change.

Thank you for your explanation!

[brophdawg11]

Dropping a link here to this more extensive explanation regarding direct usage of history/unstable_HistoryRouter: #9422 (comment)