From 5a8fc19b961ffbd02aa58a9b8cb41e0034f5aeda Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Sun, 22 Jan 2023 13:09:04 -0800 Subject: [PATCH] fix: preserve state if component disconnects then immediately reconnects (#313) Fixes #312 --- src/database/databaseLifecycle.js | 8 +-- src/picker/PickerElement.js | 33 +++++---- test/spec/picker/lifecycle.test.js | 103 ++++++++++++++++++++++++++++- 3 files changed, 125 insertions(+), 19 deletions(-) diff --git a/src/database/databaseLifecycle.js b/src/database/databaseLifecycle.js index 58d08f8..9049b4b 100644 --- a/src/database/databaseLifecycle.js +++ b/src/database/databaseLifecycle.js @@ -1,7 +1,7 @@ import { initialMigration } from './migrations' import { DB_VERSION_INITIAL, DB_VERSION_CURRENT } from './constants' -const openReqs = {} +export const openIndexedDBRequests = {} const databaseCache = {} const onCloseListeners = {} @@ -18,7 +18,7 @@ async function createDatabase (dbName) { performance.mark('createDatabase') const db = await new Promise((resolve, reject) => { const req = indexedDB.open(dbName, DB_VERSION_CURRENT) - openReqs[dbName] = req + openIndexedDBRequests[dbName] = req req.onupgradeneeded = e => { // Technically there is only one version, so we don't need this `if` check // But if an old version of the JS is in another browser tab @@ -69,7 +69,7 @@ export function dbPromise (db, storeName, readOnlyOrReadWrite, cb) { export function closeDatabase (dbName) { // close any open requests - const req = openReqs[dbName] + const req = openIndexedDBRequests[dbName] const db = req && req.result if (db) { db.close() @@ -81,7 +81,7 @@ export function closeDatabase (dbName) { } } } - delete openReqs[dbName] + delete openIndexedDBRequests[dbName] delete databaseCache[dbName] delete onCloseListeners[dbName] } diff --git a/src/picker/PickerElement.js b/src/picker/PickerElement.js index 2186797..ab07c60 100644 --- a/src/picker/PickerElement.js +++ b/src/picker/PickerElement.js @@ -48,22 +48,31 @@ export default class PickerElement extends HTMLElement { } connectedCallback () { - this._cmp = new SveltePicker({ - target: this.shadowRoot, - props: this._ctx - }) + // The _cmp may be defined if the component was immediately disconnected and then reconnected. In that case, + // do nothing (preserve the state) + if (!this._cmp) { + this._cmp = new SveltePicker({ + target: this.shadowRoot, + props: this._ctx + }) + } } disconnectedCallback () { - this._cmp.$destroy() - this._cmp = undefined + // Check in a microtask if the element is still connected. If so, treat this as a "move" rather than a disconnect + // Inspired by Vue: https://vuejs.org/guide/extras/web-components.html#building-custom-elements-with-vue + Promise.resolve().then(() => { + // this._cmp may be defined if connect-disconnect-connect-disconnect occurs synchronously + if (!this.isConnected && this._cmp) { + this._cmp.$destroy() + this._cmp = undefined - const { database } = this._ctx - if (database) { - database.close() - // only happens if the database failed to load in the first place, so we don't care) - .catch(err => console.error(err)) - } + const { database } = this._ctx + database.close() + // only happens if the database failed to load in the first place, so we don't care + .catch(err => console.error(err)) + } + }) } static get observedAttributes () { diff --git a/test/spec/picker/lifecycle.test.js b/test/spec/picker/lifecycle.test.js index 79c7330..2102ffe 100644 --- a/test/spec/picker/lifecycle.test.js +++ b/test/spec/picker/lifecycle.test.js @@ -2,6 +2,7 @@ import { basicAfterEach, basicBeforeEach, tick } from '../shared' import Picker from '../../../src/picker/PickerElement' import { getByRole, waitFor } from '@testing-library/dom' import { DEFAULT_DATA_SOURCE } from '../../../src/database/constants' +import { openIndexedDBRequests } from '../../../src/database/databaseLifecycle.js' describe('lifecycle', () => { beforeEach(basicBeforeEach) @@ -29,7 +30,8 @@ describe('lifecycle', () => { expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, { method: 'HEAD' }) document.body.removeChild(picker) - await tick(20) + await tick(60) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections }) test('database.close() is called when disconnected', async () => { @@ -42,12 +44,13 @@ describe('lifecycle', () => { const spy = jest.spyOn(picker.database, 'close') document.body.removeChild(picker) - await tick(20) + await tick(60) expect(spy).toHaveBeenCalled() expect(spy).toHaveBeenCalledTimes(1) spy.mockRestore() + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections }) test('connect and immediately disconnect', async () => { @@ -55,10 +58,25 @@ describe('lifecycle', () => { document.body.appendChild(picker) document.body.removeChild(picker) - await tick(20) + await tick(60) expect(fetch).toHaveBeenCalledTimes(1) expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections + }) + + test('connect and immediately disconnect twice', async () => { + const picker = new Picker() + document.body.appendChild(picker) + document.body.removeChild(picker) + document.body.appendChild(picker) + document.body.removeChild(picker) + + await tick(60) + + expect(fetch).toHaveBeenCalledTimes(1) + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections }) test('connect, disconnect, and reconnect with a particular timing (#225)', async () => { @@ -74,6 +92,85 @@ describe('lifecycle', () => { await tick(20) document.body.removeChild(picker) + await tick(60) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections + }) + + test('preserves state if component is disconnected and reconnected synchronously', async () => { + const picker = new Picker() + document.body.appendChild(picker) await tick(20) + + expect(fetch).toHaveBeenCalledTimes(1) + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + await expect(() => ( + expect(getByRole(picker.shadowRoot, 'option', { name: /😀/ })).toBeVisible() + )) + + document.body.removeChild(picker) + document.body.appendChild(picker) + + await tick(20) + + expect(fetch).toHaveBeenCalledTimes(1) // fetch is not called again because no re-render + await expect(() => ( + expect(getByRole(picker.shadowRoot, 'option', { name: /😀/ })).toBeVisible() + )) + + await tick(20) + document.body.removeChild(picker) + await tick(60) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections + }) + + test('does not preserve state if component is disconnected and reconnected in separate microtasks', async () => { + const picker = new Picker() + document.body.appendChild(picker) + await tick(20) + + expect(fetch).toHaveBeenCalledTimes(1) + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + await expect(() => ( + expect(getByRole(picker.shadowRoot, 'option', { name: /😀/ })).toBeVisible() + )) + + document.body.removeChild(picker) + await Promise.resolve() + document.body.appendChild(picker) + + await tick(20) + + expect(fetch).toHaveBeenCalledTimes(2) // fetch is called again due to re-render + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, { method: 'HEAD' }) // cached, so does a HEAD + await expect(() => ( + expect(getByRole(picker.shadowRoot, 'option', { name: /😀/ })).toBeVisible() + )) + + await tick(20) + document.body.removeChild(picker) + await tick(60) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections + }) + + test('connect and immediately disconnect twice, then immediately reconnect', async () => { + const picker = new Picker() + document.body.appendChild(picker) + document.body.removeChild(picker) + document.body.appendChild(picker) + document.body.removeChild(picker) + document.body.appendChild(picker) + + await tick(20) + + expect(fetch).toHaveBeenCalledTimes(1) + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + await expect(() => ( + expect(getByRole(picker.shadowRoot, 'option', { name: /😀/ })).toBeVisible() + )) + + await tick(20) + document.body.removeChild(picker) + await tick(60) + expect(Object.keys(openIndexedDBRequests).length).toBe(0) // no open IDB connections }) })