From 3749004a8ac8eddd2c60246e1243df0b18dfcfdf Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Sun, 27 Jun 2021 11:27:51 -0700 Subject: [PATCH 01/23] fix: update svelte to 3.37.0 --- config/jest.setup.js | 8 +++- package.json | 2 +- src/picker/PickerElement.js | 13 +++--- src/picker/components/Picker/Picker.js | 40 ++++++++--------- test/spec/picker/lifecycle.test.js | 59 ++++++++++++++++++++++++++ yarn.lock | 8 ++-- 6 files changed, 92 insertions(+), 38 deletions(-) create mode 100644 test/spec/picker/lifecycle.test.js diff --git a/config/jest.setup.js b/config/jest.setup.js index 0b78006e..68c67d81 100644 --- a/config/jest.setup.js +++ b/config/jest.setup.js @@ -3,6 +3,7 @@ import FDBFactory from 'fake-indexeddb/build/FDBFactory' import FDBKeyRange from 'fake-indexeddb/build/FDBKeyRange' import { Crypto } from '@peculiar/webcrypto' import { ResizeObserver } from 'd2l-resize-aware/resize-observer-module.js' +import { deleteDatabase } from '../src/database/databaseLifecycle' if (!global.performance) { global.performance = {} @@ -25,6 +26,7 @@ global.ResizeObserver = ResizeObserver process.env.NODE_ENV = 'test' global.IDBKeyRange = FDBKeyRange +global.indexedDB = new FDBFactory() beforeAll(() => { jest.spyOn(global.console, 'log').mockImplementation() @@ -32,6 +34,8 @@ beforeAll(() => { jest.spyOn(global.console, 'error').mockImplementation() }) -beforeEach(() => { - global.indexedDB = new FDBFactory() // fresh indexedDB for every test +afterEach(async () => { + // fresh indexedDB for every test + const dbs = await global.indexedDB.databases() + await Promise.all(dbs.map(({ name }) => deleteDatabase(name))) }) diff --git a/package.json b/package.json index bd05c0df..03ee1b39 100644 --- a/package.json +++ b/package.json @@ -120,7 +120,7 @@ "stylelint": "^13.13.1", "stylelint-config-recommended-scss": "^4.2.0", "stylelint-scss": "^3.19.0", - "svelte": "3.29.4", + "svelte": "3.37.0", "svelte-jester": "nolanlawson/svelte-jester#auto-preprocess", "svelte-preprocess": "^4.7.3", "svgo": "^2.3.0", diff --git a/src/picker/PickerElement.js b/src/picker/PickerElement.js index defd1dfc..ef4d3594 100644 --- a/src/picker/PickerElement.js +++ b/src/picker/PickerElement.js @@ -1,19 +1,16 @@ import SveltePicker from './components/Picker/Picker.svelte' +import { DEFAULT_DATA_SOURCE, DEFAULT_LOCALE } from '../database/constants' export default class Picker extends SveltePicker { - constructor (props) { + constructor (props = {}) { performance.mark('initialLoad') + // Set defaults + props.locale = props.locale || DEFAULT_LOCALE + props.dataSource = props.dataSource || DEFAULT_DATA_SOURCE // Make the API simpler, directly pass in the props super({ props }) } - disconnectedCallback () { - // Have to explicitly destroy the component to avoid memory leaks. - // See https://github.com/sveltejs/svelte/issues/1152 - console.log('disconnectedCallback') - this.$destroy() - } - static get observedAttributes () { return ['locale', 'data-source', 'skin-tone-emoji'] // complex objects aren't supported, also use kebab-case } diff --git a/src/picker/components/Picker/Picker.js b/src/picker/components/Picker/Picker.js index 26bb2a60..fb44ca4c 100644 --- a/src/picker/components/Picker/Picker.js +++ b/src/picker/components/Picker/Picker.js @@ -3,7 +3,6 @@ import Database from '../../ImportedDatabase' import enI18n from '../../i18n/en' import { groups as defaultGroups, customGroup } from '../../groups' -import { DEFAULT_LOCALE, DEFAULT_DATA_SOURCE } from '../../../database/constants' import { MIN_SEARCH_TEXT_LENGTH, NUM_SKIN_TONES } from '../../../shared/constants' import { requestIdleCallback } from '../../utils/requestIdleCallback' import { hasZwj } from '../../utils/hasZwj' @@ -22,7 +21,7 @@ import { summarizeEmojisForUI } from '../../utils/summarizeEmojisForUI' import * as widthCalculator from '../../utils/widthCalculator' import { checkZwjSupport } from '../../utils/checkZwjSupport' import { requestPostAnimationFrame } from '../../utils/requestPostAnimationFrame' -import { onMount, onDestroy, tick } from 'svelte' +import { onMount, tick } from 'svelte' import { requestAnimationFrame } from '../../utils/requestAnimationFrame' import { uniq } from '../../../shared/uniq' @@ -141,35 +140,30 @@ $: { } } -// TODO: this is a bizarre way to set these default properties, but currently Svelte -// renders custom elements in an odd way - props are not set when calling the constructor, -// but are only set later. This would cause a double render or a double-fetch of -// the dataSource, which is bad. Delaying with a microtask avoids this. -// See https://github.com/sveltejs/svelte/pull/4527 -onMount(async () => { - await tick() - console.log('props ready: setting locale and dataSource to default') - locale = locale || DEFAULT_LOCALE - dataSource = dataSource || DEFAULT_DATA_SOURCE +onMount(() => { + // Close the database when the component is disconnected. It will automatically reconnect anyway + // if the component is ever reconnected. + return async () => { + if (database) { + console.log('closing database') + try { + await database.close() + } catch (err) { + console.error(err) // only happens if the database failed to load in the first place, so we don't care + } + } + } }) + $: { + // API props like locale and dataSource are not actually set until the onMount phase + // https://github.com/sveltejs/svelte/pull/4522 if (locale && dataSource && (!database || (database.locale !== locale && database.dataSource !== dataSource))) { console.log('creating database', { locale, dataSource }) database = new Database({ dataSource, locale }) } } -onDestroy(async () => { - if (database) { - console.log('closing database') - try { - await database.close() - } catch (err) { - console.error(err) // only happens if the database failed to load in the first place, so we don't care - } - } -}) - // // Global styles for the entire picker // diff --git a/test/spec/picker/lifecycle.test.js b/test/spec/picker/lifecycle.test.js new file mode 100644 index 00000000..b2ea6392 --- /dev/null +++ b/test/spec/picker/lifecycle.test.js @@ -0,0 +1,59 @@ +import { basicAfterEach, basicBeforeEach, mockDefaultDataSource, tick } from '../shared' +import Picker from '../../../src/picker/PickerElement' +import { getByRole, waitFor } from '@testing-library/dom' +import { DEFAULT_DATA_SOURCE } from '../../../src/database/constants' + +describe('lifecycle', () => { + beforeEach(basicBeforeEach) + afterEach(basicAfterEach) + + test('can remove and re-append custom element', async () => { + mockDefaultDataSource() + const picker = new Picker() + document.body.appendChild(picker) + const container = picker.shadowRoot.querySelector('.picker') + + await waitFor(() => expect(getByRole(container, 'menuitem', { name: /😀/ })).toBeVisible()) + + expect(fetch).toHaveBeenCalledTimes(1) + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + + const spy = jest.spyOn(picker.database, 'close') + + document.body.removeChild(picker) + await tick(20) + + expect(spy).toHaveBeenCalled() + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockRestore() + + document.body.appendChild(picker) + await waitFor(() => expect(getByRole(container, 'menuitem', { name: /😀/ })).toBeVisible()) + + expect(fetch).toHaveBeenCalledTimes(1) + expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) + + document.body.removeChild(picker) + await tick(20) + }) + + test('database.close() is called when disconnected', async () => { + mockDefaultDataSource() + const picker = new Picker() + document.body.appendChild(picker) + const container = picker.shadowRoot.querySelector('.picker') + + await waitFor(() => expect(getByRole(container, 'menuitem', { name: /😀/ })).toBeVisible()) + + const spy = jest.spyOn(picker.database, 'close') + + document.body.removeChild(picker) + await tick(20) + + expect(spy).toHaveBeenCalled() + expect(spy).toHaveBeenCalledTimes(1) + + spy.mockRestore() + }) +}) diff --git a/yarn.lock b/yarn.lock index 47fd4c23..586d7577 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8937,10 +8937,10 @@ svelte-preprocess@^4.7.3: detect-indent "^6.0.0" strip-indent "^3.0.0" -svelte@3.29.4: - version "3.29.4" - resolved "https://registry.yarnpkg.com/svelte/-/svelte-3.29.4.tgz#d0f80cb58109ef52963855c23496f7153bb2eb7e" - integrity sha512-oW0fGHlyFFMvzRtIvOs84b0fOc0gmZNQcL5Is3hxuTpvaYX3pfd8oHy4KnOvbq4Ca6SG6AHdRMk7OhApTo0NqA== +svelte@3.37.0: + version "3.37.0" + resolved "https://registry.yarnpkg.com/svelte/-/svelte-3.37.0.tgz#dc7cd24bcc275cdb3f8c684ada89e50489144ccd" + integrity sha512-TRF30F4W4+d+Jr2KzUUL1j8Mrpns/WM/WacxYlo5MMb2E5Qy2Pk1Guj6GylxsW9OnKQl1tnF8q3hG/hQ3h6VUA== svg-tags@^1.0.0: version "1.0.0" From 6ce60a7fa9abca1005195ab75c656d98cb47b295 Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Sun, 27 Jun 2021 11:43:04 -0700 Subject: [PATCH 02/23] test: make test clearer --- test/spec/picker/lifecycle.test.js | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/test/spec/picker/lifecycle.test.js b/test/spec/picker/lifecycle.test.js index b2ea6392..a242f8f5 100644 --- a/test/spec/picker/lifecycle.test.js +++ b/test/spec/picker/lifecycle.test.js @@ -10,27 +10,22 @@ describe('lifecycle', () => { test('can remove and re-append custom element', async () => { mockDefaultDataSource() const picker = new Picker() - document.body.appendChild(picker) const container = picker.shadowRoot.querySelector('.picker') + document.body.appendChild(picker) + await waitFor(() => expect(getByRole(container, 'menuitem', { name: /😀/ })).toBeVisible()) expect(fetch).toHaveBeenCalledTimes(1) expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) - const spy = jest.spyOn(picker.database, 'close') - document.body.removeChild(picker) await tick(20) - expect(spy).toHaveBeenCalled() - expect(spy).toHaveBeenCalledTimes(1) - - spy.mockRestore() - document.body.appendChild(picker) await waitFor(() => expect(getByRole(container, 'menuitem', { name: /😀/ })).toBeVisible()) + // fetches are unchanged, no new fetches after re-insertion expect(fetch).toHaveBeenCalledTimes(1) expect(fetch).toHaveBeenLastCalledWith(DEFAULT_DATA_SOURCE, undefined) From 55d12b1ebd1ad06a442be06924770e8a62f27710 Mon Sep 17 00:00:00 2001 From: Nolan Lawson Date: Sun, 27 Jun 2021 14:39:51 -0700 Subject: [PATCH 03/23] fix: fix memory leak --- src/picker/components/Picker/Picker.html | 5 ++--- src/picker/components/Picker/Picker.js | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/picker/components/Picker/Picker.html b/src/picker/components/Picker/Picker.html index 27957647..43b6656b 100644 --- a/src/picker/components/Picker/Picker.html +++ b/src/picker/components/Picker/Picker.html @@ -98,7 +98,7 @@
+ bind:this={indicatorElement}>
@@ -134,8 +134,7 @@
+ id={searchMode ? 'search-results' : ''}> {#each emojiWithCategory.emojis as emoji, i (emoji.id)} - {/each} -
- {/each} +
+ {#each currentEmojisWithCategories as emojiWithCategory, i (emojiWithCategory.category)} + +
+ {#each emojiWithCategory.emojis as emoji, i (emoji.id)} + + {/each} +
+ {/each} +