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 87725d7. * fix: Revert "chore: test old svelte version" This reverts commit f6431f2. * 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
nolanlawson · Jun 29, 2021 · 50ac48d · 50ac48d
1 parent f5e9dcb
commit 50ac48d
Show file tree

Hide file tree

Showing 11 changed files with 192 additions and 91 deletions.
diff --git 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
diff --git 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

diff --git 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,13 +26,16 @@ global.ResizeObserver = ResizeObserver
 process.env.NODE_ENV = 'test'
 
 global.IDBKeyRange = FDBKeyRange
+global.indexedDB = new FDBFactory()
 
 beforeAll(() => {
   jest.spyOn(global.console, 'log').mockImplementation()
   jest.spyOn(global.console, 'warn').mockImplementation()
   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
@@ -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"
     },
     {

diff --git 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)
diff --git 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"

diff --git 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
 //

diff --git a/src/picker/utils/runAll.js b/src/picker/utils/runAll.js
@@ -0,0 +1 @@
+export const runAll = funcs => (funcs && funcs.forEach(func => func()))
diff --git 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()
     }
   }
 }