Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(routing): fix history router based on history length #5004

Merged
merged 12 commits into from
Feb 7, 2022

Conversation

FabienMotte
Copy link
Contributor

@FabienMotte FabienMotte commented Feb 2, 2022

Summary

This PR solves the issue with the history router when the InstantSearch.js is disposed:

  • in a SPA use case, the URL should not be cleaned.
  • in a modal use case, the URL should be cleaned.

The fix has been suggested by @Haroenv and it's based on the window.history.length. It allows to determine if a window.history.pushState has been triggered elsewhere, and thus to prevent the write (called when dispose is called on the InstantSearch.js instance) method from calling window.history.pushState.

Result

As a result, in a SPA use case, the history stack is correct and the browser back button works as expected. In a modal use case, when the InstantSearch.js instance is disposed, the URL is cleaned as expected.

SPA use case: https://codesandbox.io/s/vue-instantsearch-spa-fixed-fsdno
Modal use case (not changed): https://codesandbox.io/s/instantsearch-js-modal-zg12q

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 2, 2022

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 de02dca:

Sandbox Source
InstantSearch.js Configuration
vue-instantsearch_spa_fixed PR
instantsearch.js_modal PR

src/lib/routers/history.ts Outdated Show resolved Hide resolved
src/lib/routers/history.ts Outdated Show resolved Hide resolved
src/lib/routers/history.ts Outdated Show resolved Hide resolved
src/lib/routers/history.ts Outdated Show resolved Hide resolved
src/lib/routers/history.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/spa-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/spa-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/spa-debounced-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/external-influence-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/modal-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/spa-debounced-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/spa-replace-state-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/spa-test.ts Outdated Show resolved Hide resolved
@FabienMotte
Copy link
Contributor Author

Thanks for your reviews!

I think it makes sense to document these two behaviors @sarahdayan mentioned:

Routing with debounced (more than 400 ms) third-party client-side router does not clean the URL after navigating

The scenario as I understand it is:

  • User refines InstantSearch, route updates (/?indexName[query]=Apple)
  • User clicks navigation handled by 3rd party router, but router doesn't kick in right away because it's debounced (longer delay than ours)
  • InstantSearch is disposed, history length is the same, so it cleans (/)
  • Router finally writes (/about)

If yes I think this is clear, but we should probably document it because having this intermediary / route can be surprising.

Routing using replaceState cleans the URL after navigating

Yup we do it in the next step.

Is this a behavior worth documenting? Should we say that our router doesn't work well with other routers that use replaceState?

WDYT?

@Haroenv
Copy link
Contributor

Haroenv commented Feb 4, 2022

Where would you document that behaviour? https://www.algolia.com/doc/api-reference/widgets/history-router/js/#about ?

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎸

@FabienMotte
Copy link
Contributor Author

Where would you document that behaviour? https://www.algolia.com/doc/api-reference/widgets/history-router/js/#about ?

I think it could make sense in that page indeed, WDYT @sarahdayan?

src/lib/__tests__/routing/dispose-start-test.ts Outdated Show resolved Hide resolved
src/lib/__tests__/routing/dispose-start-test.ts Outdated Show resolved Hide resolved
@sarahdayan
Copy link
Member

@FabienMotte Yes, we can document it here and have warnings that link to it in the routing guides that use the historyrouter:

@sarahdayan sarahdayan changed the title fix(routing): fix history router based on history length fix(routing): fix history router based on history length Feb 7, 2022
@sarahdayan sarahdayan changed the title fix(routing): fix history router based on history length fix(routing): fix history router based on history length Feb 7, 2022
@FabienMotte FabienMotte merged commit 40541af into master Feb 7, 2022
@FabienMotte FabienMotte deleted the fix/routing-history-length branch February 7, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants