feat(routing): new router option to prevent write on dispose

[FabienMotte]

Summary

This PR adds an option called writeOnDispose to the history router. It allows to whether call the write method on dispose or not. By default, the option is set to true.

This option allows to prevent write on dispose, so window.history.pushState is not called when the InstantSearch instance is disposed (when you go to another page for example).

https://github.com/algolia/instantsearch.js/blob/45433de9e21801f8b0c6d0859cd4e6aacb666104/src/lib/routers/history.ts#L198-L202

Also, in the router middleware, write method in onStateChange is prevented if the middleware gets unsubscribed.

https://github.com/algolia/instantsearch.js/blob/45433de9e21801f8b0c6d0859cd4e6aacb666104/src/middlewares/createRouterMiddleware.ts#L71-L78

For more context, you can read the related PR: #4808

Result

As a consequence, the browser "Go back" button should work as expected when writeOnDispose is set to false.

writeOnDispose=false

write_on_dispose_false.mov

writeOnDispose=true (default behavior)

write_on_dispose_true.mov

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1f01566:

Sandbox	Source
InstantSearch.js	Configuration

[Haroenv]

this was a long time coming, and I'm happy we'll finally have a better solution than "copy the router" for this feature

[Haroenv]

I'd like to find a better name, but can't think of anything

• cleanup: true/false
• cleanupOnDispose: true/false
• singlepage: false/true (not really a fan)
• externalRouter: false/true (not really a fan)

[sarahdayan]

@Haroenv What don't you like about it? What meaning should it convey that you think is missing or improperly expressed right now?

[Haroenv]

for me it seems an implementation detail, I'd rather have the option explain why you'd want to do that in the first place

[FabienMotte]

Should we add a paragraph in the docs to explain this new option (and keep the name as is it)?

[sarahdayan]

What does this prevent? I've ran the tests without this line and nothing breaks.

We likely need to test this behavior if this is actually needed.

[francoischalifour]

Why shouldn't this be the default behavior?

If we find scenarios where this behavior as a default causes problems, I feel like we can find a sensible default without introducing an option.

[Haroenv]

In regular InstantSearch.js, it probably should default to the current behaviour, as on dispose, the InstantSearch is no longer valid, and thus should return the URL to its previous state.

Maybe that shouldn't be the default, but changing it is breaking

[francoischalifour]

I think that keeping the URL on dispose() in all flavors may be a good strategy, because:

1. the next UI state change will restore the URL
2. if users need to clean the URL, they can manipulate the URL themselves (no API), or call a method on the router (different API: a method and not an option)

I'm wondering if (2) even happens. How many users call dispose() in InstantSearch.js? Is it a problem for them that the URL stays unchanged? Do they even notice?

I'm not sure whether this slight behavior change is a breaking change. Do you think that it could break users' apps? tests?

[Haroenv]

I'm not sure if anyone expects that behaviour on purpose, but InstantSearch has always cleaned/cleared the url on dispose. People might have a ui where InstantSearch is conditional, and when it unmounts, while still on the same page, would expect to get back to a clean URL?

Again, not sure if this matters, but I'd edge on the side of caution

[FabienMotte]

Superseded by: #5004