fix: update svelte to 3.37.0 (#152)

* fix: update svelte to 3.37.0 * test: make test clearer * fix: fix memory leak * fix: fix alignment in firefox * test: bump bundlesize limit * chore: test old svelte version * fix: fix for old svelte * fix: Revert "fix: fix for old svelte" This reverts commit 87725d713ee5b026e7e09bebb3f9df8dbbe23097. * fix: Revert "chore: test old svelte version" This reverts commit f6431f29d40155b984f4784b1237f72909844101. * chore: test old svelte version * test: test more svelte * test: test svelte 3.29.4 * fix: fix for older versions of Svelte * test: fix tested svelte versions * test: fix ordering * test: fix ordering again * test: bump bundlesize * test: remove unnecessary test * test: test that the tests actually work in Circle CI * fix: un-break test * fix: simplify the code a bit * docs: readme * fix: refactor shared logic
2021-06-28 20:31:44 -07:00 · 2021-06-28 20:31:44 -07:00 · 50ac48d4c8
parent f5e9dcbfe3
commit 50ac48d4c8
11 changed files with 192 additions and 91 deletions
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@ -1,12 +1,21 @@
 version: 2.1

+supported-svelte-versions: &supported-svelte-versions ["local", "3.29.4"]
+
 workflows:
-  version: 2
-  build:
+  build_and_test:
    jobs:
-      - build_and_test
+      - build_and_test:
+          matrix:
+            parameters:
+              svelte-version: *supported-svelte-versions
 jobs:
  build_and_test:
+    parameters:
+      svelte-version:
+        type: string
+        description: Overrides the Svelte version. `local` refers to the locally-installed version.
+        default: "local"
    docker:
      - image: circleci/node:14-buster-browsers
    steps:
@ -25,9 +34,27 @@ jobs:
      - run:
          name: Yarn install
          command: yarn install --immutable
+      - save_cache:
+          name: Save yarn cache
+          key: yarn-v1-{{ checksum "yarn.lock" }}
+          paths:
+            - ~/.cache/yarn
      - run:
          name: Lint
          command: yarn lint
+      - run:
+          name: Bundlesize tests
+          command: yarn benchmark:bundlesize
+      - when:
+          condition:
+            not:
+              equal: [ <<parameters.svelte-version>>, "local" ]
+          steps:
+            - run:
+                name: Override version of svelte@<<parameters.svelte-version>>
+                # Do --ignore-scripts because we don't actually want the old version of Svelte to compile
+                # the picker; just get injected at runtime. This is how `emoji-picker-element/svelte` is used.
+                command: yarn add svelte@<<parameters.svelte-version>> --dev --ignore-scripts
      - run:
          name: Unit tests
          command: yarn test
@ -37,13 +64,5 @@ jobs:
      - run:
          name: Leak tests
          command: yarn test:leak
-      - run:
-          name: Bundlesize tests
-          command: yarn benchmark:bundlesize
-      - save_cache:
-          name: Save yarn cache
-          key: yarn-v1-{{ checksum "yarn.lock" }}
-          paths:
-            - ~/.cache/yarn
      - store_artifacts:
          path: coverage
--- a/README.md
+++ b/README.md
@ -741,6 +741,8 @@ import Picker from 'emoji-picker-element/svelte';

 `svelte.js` is the same as `picker.js`, except it `import`s Svelte rather than bundling it.

+While this option can reduce your bundle size, note that it only works if your Svelte version is compatible with `emoji-picker-element`'s Svelte version. You can check [the tests](https://github.com/nolanlawson/emoji-picker-element/blob/master/.circleci/config.yml) to see which Svelte versions are tested.
+
 ## Data and offline

 ### Data source and JSON format
--- 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)))
 })
--- a/package.json
+++ b/package.json
@ -33,7 +33,7 @@
    "benchmark:run-bundlesize": "bundlesize",
    "benchmark:storage": "cross-env PERF=1 run-s build:rollup && run-p --race test:adhoc benchmark:storage:test",
    "benchmark:storage:test": "node ./test/storage/test.js",
-    "test:leak": "run-s build:rollup && run-p --race test:leak:server test:leak:test",
+    "test:leak": "run-p --race test:leak:server test:leak:test",
    "test:leak:server": "node ./test/leak/server.js",
    "test:leak:test": "node ./test/leak/test.js",
    "dev": "run-p --race dev:rollup dev:server",
@ -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",
@ -189,7 +189,7 @@
  "bundlesize": [
    {
      "path": "./bundle.js",
-      "maxSize": "41.2 kB",
+      "maxSize": "41.4 kB",
      "compression": "none"
    },
    {
--- a/src/picker/PickerElement.js
+++ b/src/picker/PickerElement.js
@ -1,17 +1,29 @@
 import SveltePicker from './components/Picker/Picker.svelte'
+import { DEFAULT_DATA_SOURCE, DEFAULT_LOCALE } from '../database/constants'
+import { runAll } from './utils/runAll'

-export default class Picker extends SveltePicker {
+export default class PickerElement extends SveltePicker {
  constructor (props) {
    performance.mark('initialLoad')
    // Make the API simpler, directly pass in the props
-    super({ props })
+    super({
+      props: {
+        // Set defaults
+        locale: DEFAULT_LOCALE,
+        dataSource: DEFAULT_DATA_SOURCE,
+        ...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()
+    // For Svelte v <3.33.0, we have to run the destroy logic ourselves because it doesn't have this fix:
+    // https://github.com/sveltejs/svelte/commit/d4f98f
+    // We can safely just run on_disconnect and on_destroy to cover all versions of Svelte. In older versions
+    // the on_destroy array will have length 1, whereas in more recent versions it'll be on_disconnect instead.
+    // TODO: remove this when we drop support for Svelte < 3.33.0
+    runAll(this.$$.on_destroy)
+    runAll(this.$$.on_disconnect)
  }

  static get observedAttributes () {
@ -28,4 +40,4 @@ export default class Picker extends SveltePicker {
  }
 }

-customElements.define('emoji-picker', Picker)
+customElements.define('emoji-picker', PickerElement)
--- a/src/picker/components/Picker/Picker.html
+++ b/src/picker/components/Picker/Picker.html
@ -98,7 +98,7 @@
  <div class="indicator-wrapper">
    <div class="indicator"
         style={indicatorStyle}
-         use:calculateIndicatorWidth>
+         bind:this={indicatorElement}>
    </div>
  </div>

@ -117,41 +117,42 @@
       on:click={onEmojiClick}
       bind:this={tabpanelElement}
  >
-    {#each currentEmojisWithCategories as emojiWithCategory, i (emojiWithCategory.category)}
-      <div
-        id="menu-label-{i}"
-        class="category {currentEmojisWithCategories.length > 1 ? '' : 'gone'}"
-        aria-hidden="true">
-        <!-- This logic is a bit complicated in order to avoid a flash of the word "Custom" while switching
-             from a tabpanel with custom emoji to a regular group. I.e. we don't want it to suddenly flash
-             from "Custom" to "Smileys and emoticons" when you click the second nav button. -->
-        {searchMode ? i18n.searchResultsLabel : (
-          emojiWithCategory.category ? emojiWithCategory.category : (
-            currentEmojisWithCategories.length > 1 ? i18n.categories.custom : i18n.categories[currentGroup.name]
-          )
-        )}
-      </div>
-      <div class="emoji-menu"
-           role={searchMode ? 'listbox' : 'menu'}
-           aria-labelledby="menu-label-{i}"
-           id={searchMode ? 'search-results' : ''}
-           use:calculateEmojiGridWidth>
-        {#each emojiWithCategory.emojis as emoji, i (emoji.id)}
-          <button role={searchMode ? 'option' : 'menuitem'}
-                  aria-selected={searchMode ? i == activeSearchItem : ''}
-                  aria-label={labelWithSkin(emoji, currentSkinTone)}
-                  title={emoji.title}
-                  class="emoji {searchMode && i === activeSearchItem ? 'active' : ''}"
-                  id="emo-{emoji.id}">
-            {#if emoji.unicode}
-              {unicodeWithSkin(emoji, currentSkinTone)}
-            {:else}
-              <img class="custom-emoji" src={emoji.url} alt="" loading="lazy" />
-            {/if}
-          </button>
-        {/each}
-      </div>
-    {/each}
+    <div bind:this={tabpanelInnerElement}>
+      {#each currentEmojisWithCategories as emojiWithCategory, i (emojiWithCategory.category)}
+        <div
+          id="menu-label-{i}"
+          class="category {currentEmojisWithCategories.length > 1 ? '' : 'gone'}"
+          aria-hidden="true">
+          <!-- This logic is a bit complicated in order to avoid a flash of the word "Custom" while switching
+               from a tabpanel with custom emoji to a regular group. I.e. we don't want it to suddenly flash
+               from "Custom" to "Smileys and emoticons" when you click the second nav button. -->
+          {searchMode ? i18n.searchResultsLabel : (
+            emojiWithCategory.category ? emojiWithCategory.category : (
+              currentEmojisWithCategories.length > 1 ? i18n.categories.custom : i18n.categories[currentGroup.name]
+            )
+          )}
+        </div>
+        <div class="emoji-menu"
+             role={searchMode ? 'listbox' : 'menu'}
+             aria-labelledby="menu-label-{i}"
+             id={searchMode ? 'search-results' : ''}>
+          {#each emojiWithCategory.emojis as emoji, i (emoji.id)}
+            <button role={searchMode ? 'option' : 'menuitem'}
+                    aria-selected={searchMode ? i == activeSearchItem : ''}
+                    aria-label={labelWithSkin(emoji, currentSkinTone)}
+                    title={emoji.title}
+                    class="emoji {searchMode && i === activeSearchItem ? 'active' : ''}"
+                    id="emo-{emoji.id}">
+              {#if emoji.unicode}
+                {unicodeWithSkin(emoji, currentSkinTone)}
+              {:else}
+                <img class="custom-emoji" src={emoji.url} alt="" loading="lazy" />
+              {/if}
+            </button>
+          {/each}
+        </div>
+      {/each}
+    </div>
  </div>
  <div class="favorites emoji-menu {message ? 'gone': ''}"
       role="menu"
--- 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,9 +21,10 @@ 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'
+import { runAll } from '../../utils/runAll'

 // public
 let locale = null
@ -44,6 +44,8 @@ let searchText = ''
 let rootElement
 let baselineEmoji
 let tabpanelElement
+let tabpanelInnerElement
+let indicatorElement
 let searchMode = false // eslint-disable-line no-unused-vars
 let activeSearchItem = -1
 let message // eslint-disable-line no-unused-vars
@ -141,35 +143,42 @@ $: {
  }
 }

-// 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(() => {
+  const destroys = [
+    calculateIndicatorWidth(indicatorElement),
+    // The reason for the tabpanelInnerElement is that, if we measure the width on the tabpanelElement,
+    // then we don't always exclude the scrollbar. In Chrome/WebKit it does, in Firefox it does not.
+    calculateEmojiGridWidth(tabpanelInnerElement)
+  ]
+
+  return async () => {
+    // TODO: using a workaround for Svelte actions never calling destroy() when used in
+    // custom elements. Instead of waiting for a destroy event, we use the mount/unmount
+    // lifecycle to clean up.
+    // https://github.com/sveltejs/svelte/issues/5989#issuecomment-796366910
+    runAll(destroys)
+    // Close the database when the component is disconnected. It will automatically reconnect anyway
+    // if the component is ever reconnected.
+    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
 //
--- a/src/picker/utils/runAll.js
+++ b/src/picker/utils/runAll.js
@ -0,0 +1 @@
+export const runAll = funcs => (funcs && funcs.forEach(func => func()))
--- a/src/picker/utils/widthCalculator.js
+++ b/src/picker/utils/widthCalculator.js
@ -24,11 +24,10 @@ export function calculateWidth (node, onUpdate) {
    ))
  }

-  return {
-    destroy () {
-      if (resizeObserver) {
-        resizeObserver.disconnect()
-      }
+  // cleanup function (called on disconnect)
+  return () => {
+    if (resizeObserver) {
+      resizeObserver.disconnect()
    }
  }
 }
--- a/test/spec/picker/lifecycle.test.js
+++ b/test/spec/picker/lifecycle.test.js
@ -0,0 +1,54 @@
+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()
+    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)
+
+    document.body.removeChild(picker)
+    await tick(20)
+
+    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)
+
+    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()
+  })
+})
--- 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"
				`@ -0,0 +1 @@`
				`export const runAll = funcs => (funcs && funcs.forEach(func => func()))`