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

feat: Sync local storage state across tabs when out of sync #4545

Merged
merged 28 commits into from
Jan 27, 2022

Conversation

ad1992
Copy link
Member

@ad1992 ad1992 commented Jan 6, 2022

sync

An attempt to fix #2791

  • Sync elements
  • Sync appstate
  • sync username
  • sync language
  • sync library

@vercel
Copy link

vercel bot commented Jan 6, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/HWxvE9bWPfNYYXM9Dt6bjBijwbRY
✅ Preview: https://excalidraw-git-aakansha-sync-excalidraw.vercel.app

src/components/App.tsx Outdated Show resolved Hide resolved
@dwelle
Copy link
Member

dwelle commented Jan 10, 2022

I've also added window focus event. This will catch cases of having both tabs visible or partially visible.

@dwelle
Copy link
Member

dwelle commented Jan 10, 2022

The failing tests were caused because blurring the wysiwyg was for some reason firing change event on window, which in turn overwritten the elements with empty [] because in tests we don't persist state to LS.

But this makes me wonder — the change event we're now listening for will be triggered in many cases like picking a file from the filesystem, thus potentially overwriting the state with stale data in case the LS isn't updated in time. This could happen, since dialogs are generally thread blocking. Also could happen once we move to IDB which is async.

Also note that even visibilitychange alone could be affected by this.

@dwelle
Copy link
Member

dwelle commented Jan 10, 2022

Another, related thing: we need to disable the syncing when collaborating, because we're not syncing to LS during collab.

@ad1992
Copy link
Member Author

ad1992 commented Jan 12, 2022

I've also added window focus event. This will catch cases of having both tabs visible or partially visible.

Why do we need focus event ? coz this will trigger LS sync when ever window gains focus even when visible eg switching focus from devtools to Excalidraw window, also the visibilityChange will be triggered when ever the page becomes fully visible from partial visible/minimised state as well.

@dwelle
Copy link
Member

dwelle commented Jan 12, 2022

Why do we need focus event ? coz this will trigger LS sync when ever window gains focus even when visible eg switching focus from devtools to Excalidraw window, also the visibilityChange will be triggered when ever the page becomes fully visible from partial visible/minimised state as well.

visibilityChange will give us only a subset of focus (except for one special case). We want to handle cases like when you have both tabs visible (on one or two monitors) and switch between them.

Either way, we want to make the syncing robust enough so we don't have to think whether change event can cause race conditions. It must work properly no matter what.

# Conflicts:
#	src/tests/binding.test.tsx
- switching tabs before changes saved to LS (doesn't fix IDB)
- makes collaboration local-state fix (previous commit) more
  deterministic (otherwise we'd have to hope that the sync isn't called
  before we reset the verisons)
@dwelle dwelle merged commit ca89d47 into master Jan 27, 2022
@dwelle dwelle deleted the aakansha-sync branch January 27, 2022 12:21
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.

Sync state between tabs on each change
2 participants