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

reset store without reconnecting socket #898

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andy-k
Copy link
Collaborator

@andy-k andy-k commented Jan 22, 2022

partial attempt at #877.

liwords-socket needs the path to know which realms to subscribe to. if the path changes, the socket must be killed and reconnected.

on closer inspection, it seems only paths starting with /game/, /tournament/, or /club/ need to be preserved.

this PR

  • moves LiwordsSocket to between LoginStateStore and RestOfStore.
  • changes the reset whole store on path change to reset only RestOfStore, so that in itself this does not cause socket reconnection.
  • removes path from LoginStateStore. it's only used in TournamentRoom and is currently always equal to the path returned by react-router-dom anyway. also removes isChild, which is unused. (there is a no-op cast in store/login_state.ts that seems to not prevent injection of such additional keys during the object spread.)
  • canonicalizes, for the purpose of socket realm subscription, all paths not starting with /game/, /tournament/, or /club/ to /. so, there's no reconnection when people go from / to /profile to /settings to /team or something like that. (of course it's not very useful if the main load is from rematch redirects.)
  • late-binds the socket handler to accommodate the above change. while the socket itself only depends on the login state and canonicalized path, the handler called when a socket message arrives processes that message on the rest of store.
  • does nothing about auto socket reconnection, it seems there's already some logic there.

expect even more bugs. test thoroughly and be ready to revert.

obviously, this PR introduces a new bug: socket messages received by the old handler will be processed by the old rest of store, and socket messages received by the new handler will be processed by the new rest of store after reset. (there's also a brief period of time when neither handler is installed, during which received messages are lost permanently.) as long as "reset rest of store" is still a thing, this bug cannot be fixed.

@andy-k andy-k requested a review from domino14 January 22, 2022 10:18
@domino14
Copy link
Collaborator

Please rebase when you get the chance; I believe both sets of imports are required now? (in the file with conflicts)

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.

2 participants