Better handle multiple calls to mount (#2123)

To help better handle cases where mount may be called multiple times, this PR better handles entry creation and re-registration based on whether or not the entry has already been registered.

packages/core/src/js/Collection.js

@@ -1,4 +1,4 @@
-import { eventEmitter, maybeRunMethod } from "./utilities";
+import { eventEmitter, getElement, maybeRunMethod } from "./utilities";
 import { dispatchLifecycleHook, pluginsArray } from "./helpers";
 import { CollectionEntry } from "./CollectionEntry";
 
@@ -66,23 +66,37 @@ export class Collection {
   }
 
   async register(query, config = {}) {
-    // Create the collection entry object and mount it.
-    const entry = await this.createEntry(query, config);
+    // Get the element to register.
+    const element = getElement(query);
 
-    // Check if an entry with the provided ID has already been registered.
-    const index = this.collection.findIndex(item => item.id === entry.id);
+    // Throw an error if element wasn't found.
+    if (element === null) {
+      throw new Error(`${this.module} element was not found with ID: "${query?.id || query}"`);
+    }
+
+    // Check if the element with the provided ID has already been registered.
+    const index = this.collection.findIndex(item => item.id === element.id);
     if (~index) {
-      // Replace the existing entry in the collection.
-      this.collection[index] = entry;
+      // Get the entry from the collection.
+      const entry = this.collection[index];
+      // Override the element property with the provided element.
+      entry.el = element;
+      // Run the entry init() method if it exists.
+      if (typeof entry.init === "function") {
+        await entry.init(config);
+      }
+      // Return the registered entry.
+      return entry;
     } else {
+      // Create the collection entry object.
+      const entry = await this.createEntry(element, config);
       // Add the entry to the collection.
       this.collection.push(entry);
+      // Dispatch onRegisterEntry lifecycle hooks.
+      await dispatchLifecycleHook("onRegisterEntry", this, entry);
+      // Return the registered entry.
+      return entry;
     }
-
-    // Dispatch onRegisterEntry lifecycle hooks.
-    await dispatchLifecycleHook("onRegisterEntry", this, entry);
-
-    return entry;
   }
 
   async deregister(id) {

packages/core/tests/plugins/propStore.test.js

@@ -7,139 +7,179 @@ document.body.innerHTML = `
   <div class="entry" id="entry-3">Three</div>
 `;
 
-const collection = new Collection({
-  selector: ".entry"
-});
+let collection;
 
-test("should register and setup the propStore plugin on collection mount", async () => {
-  expect(collection.plugins.length).toBe(0);
-  await collection.mount({
-    plugins: [
-      propStore()
-    ]
+describe("propStore", () => {
+  beforeEach(() => {
+    collection = new Collection({
+      selector: ".entry"
+    });
+  });
+
+  afterEach(async () => {
+    await collection.unmount();
   });
-  expect(collection.plugins.length).toBe(1);
-  const entry = collection.get("entry-1");
-  const plugin = collection.plugins.get("propStore");
-  expect(entry.id).toBe("entry-1");
-  expect(typeof plugin.store.get).toBe("function");
-  expect(typeof plugin.store.set).toBe("function");
-  expect(typeof plugin.settings.onChange).toBe("function");
-  expect(typeof plugin.settings.condition).toBe("boolean");
-  plugin.settings.onChange();
-  expect(plugin.settings.condition).toBe(false);
-});
 
-test("should remove the propStore plugin when the remove method is called", async () => {
-  expect(collection.plugins.length).toBe(1);
-  await collection.plugins.remove("propStore");
-  expect(collection.plugins.length).toBe(0);
-});
+  it("should register and setup the propStore plugin on collection mount", async () => {
+    expect(collection.plugins.length).toBe(0);
+    await collection.mount({ plugins: [ propStore() ] });
+    expect(collection.plugins.length).toBe(1);
+    const entry = collection.get("entry-1");
+    const plugin = collection.plugins.get("propStore");
+    expect(entry.id).toBe("entry-1");
+    expect(typeof plugin.store.get).toBe("function");
+    expect(typeof plugin.store.set).toBe("function");
+    expect(typeof plugin.settings.onChange).toBe("function");
+    expect(typeof plugin.settings.condition).toBe("boolean");
+    await plugin.settings.onChange();
+    expect(plugin.settings.condition).toBe(false);
+  });
 
-test("should remove propStore from an entry if it is deregistered", async () => {
-  await collection.mount({
-    plugins: [
-      propStore({
-        name: "quickPropStore"
-      })
-    ]
+  it("should remove the propStore plugin when the remove method is called", async () => {
+    expect(collection.plugins.length).toBe(0);
+    await collection.mount({ plugins: [propStore()] });
+    expect(collection.plugins.length).toBe(1);
+    await collection.plugins.remove("propStore");
+    expect(collection.plugins.length).toBe(0);
   });
-  const entry = collection.get("entry-1");
-  await collection.deregister("entry-1");
-  expect(entry).toEqual({ "id": "entry-1" });
-  await collection.register("entry-1");
-  const el = document.getElementById("entry-1");
-  expect(collection.get("entry-1").el).toBe(el);
-});
 
-test("should be able to set a condition and onChange callback", async () => {
-  const spyFunction = vi.fn();
-  await collection.mount({
-    plugins: [
-      propStore({
-        prop: "example",
-        condition() {
-          return true;
-        },
-        onChange: spyFunction
-      })
-    ]
+  it("should remove propStore from an entry if it is deregistered", async () => {
+    // Initial setup for propStore.
+    await collection.mount({
+      plugins: [
+        propStore({
+          prop: "example",
+          value: "asdf",
+          condition: true
+        })
+      ]
+    });
+
+    // Get a reference to the plugin's local store object.
+    const store = collection.plugins.get("propStore").store;
+
+    const entry = collection.get("entry-1");
+    // Check that the property and initial value has been added to entry.
+    expect(entry.example).toBe("asdf");
+    // Check that the value of entry has been stored in local storage.
+    expect(store.get("entry-1")).toBe("asdf");
+
+    // Deregister the entry from the collection.
+    await collection.deregister("entry-1");
+
+    // Check that the property has been removed.
+    expect(entry.example).toBe(undefined);
+    // Check that the value of entry has been removed from local storage.
+    expect(store.get("entry-1")).toBe(undefined);
+
+    // Ensure that the other entries still have their values stored in local storage.
+    expect(store.get("entry-2")).toBe("asdf");
+    expect(store.get("entry-3")).toBe("asdf");
   });
-  const entry = collection.get("entry-1");
-  expect(spyFunction).toBeCalledTimes(0);
-  entry.example = "fdsa";
-  expect(entry.example).toBe("fdsa");
-  expect(spyFunction).toBeCalledTimes(1);
-});
 
-test("should not fire onChange callback if setting a value that isn't different", async () => {
-  const entry = collection.get("entry-2");
-  entry.example = "asdf";
-  expect(entry.example).toBe("asdf");
-  entry.example = "asdf";
-});
+  it("should be able to set a condition and onChange callback", async () => {
+    const spyFunction = vi.fn();
+    await collection.mount({
+      plugins: [
+        propStore({
+          prop: "example",
+          condition() {
+            return true;
+          },
+          onChange: spyFunction
+        })
+      ]
+    });
+    const entry = collection.get("entry-1");
+    expect(spyFunction).toBeCalledTimes(0);
+    entry.example = "fdsa";
+    expect(entry.example).toBe("fdsa");
+    expect(spyFunction).toBeCalledTimes(1);
+  });
 
-test("should setup a store property for accessing the local storage value", async () => {
-  const entry = collection.get("entry-3");
-  entry.example = "test";
-  expect(entry.store).toBe("test");
-  entry.store = "new";
-  entry.example = "new";
-  expect(entry.store).toBe("new");
-});
+  it("should not fire onChange callback if setting a value that isn't different", async () => {
+    const spyFunction = vi.fn();
+    await collection.mount({
+      plugins: [
+        propStore({
+          prop: "example",
+          condition() {
+            return true;
+          },
+          onChange: spyFunction
+        })
+      ]
+    });
+    const entry = collection.get("entry-1");
+
+    // Ensure onChange has not been called.
+    expect(spyFunction).toBeCalledTimes(0);
+
+    // Set the value of the prop and ensure onChange is called once.
+    entry.example = "asdf";
+    expect(spyFunction).toBeCalledTimes(1);
+
+    // Set the value again but ensure onChange is NOT called again since the value has not changed.
+    entry.example = "asdf";
+    expect(spyFunction).toBeCalledTimes(1);
 
-test("should be able to set an initial value for the property", async() => {
-  const spyFunction = vi.fn();
-  expect(collection.plugins.length).toBe(2);
-  await collection.mount({
-    plugins: [
-      propStore({
-        name: "newPropStore",
-        prop: "hello",
-        value: "world",
-        condition: () => true,
-        onChange: spyFunction
-      })
-    ]
+    // Ensure that setting the prop to a different value does fire onChange.
+    entry.example = "fdsa";
+    expect(spyFunction).toBeCalledTimes(2);
   });
-  expect(collection.plugins.length).toBe(3);
-  const entry = collection.get("entry-1");
-  expect(entry.hello).toBe("world");
-  expect(spyFunction).toBeCalledTimes(3);
-  entry.hello = "buddy";
-  expect(spyFunction).toBeCalledTimes(4);
-});
 
-test("should be able to set an initial value using a function definition", async() => {
-  expect(collection.plugins.length).toBe(3);
-  await collection.mount({
-    plugins: [
-      propStore({
-        name: "anotherNewPropStore",
-        prop: "hello",
-        value: () => "my" + " " + "friend",
-        condition: () => true
-      })
-    ]
+  it("should setup a store property for accessing the local storage value", async () => {
+    await collection.mount({ 
+      plugins: [
+        propStore({
+          prop: "example",
+          condition: true
+        })
+      ]
+    });
+    const entry = collection.get("entry-3");
+
+    // Setting the property directly should update store.
+    entry.example = "test";
+    expect(entry.store).toBe("test");
+    expect(entry.example).toBe("test");
+
+    // Setting the store value directly should update the property.
+    entry.store = "new";
+    expect(entry.store).toBe("new");
+    expect(entry.example).toBe("new");
+  });
+
+  it("should be able to set an initial value for the property", async() => {
+    const spyFunction = vi.fn();
+    await collection.mount({
+      plugins: [
+        propStore({
+          prop: "hello",
+          value: "world",
+          condition: () => true,
+          onChange: spyFunction
+        })
+      ]
+    });
+    const entry = collection.get("entry-1");
+    expect(entry.hello).toBe("world");
+    expect(spyFunction).toBeCalledTimes(3);
+    entry.hello = "buddy";
+    expect(spyFunction).toBeCalledTimes(4);
   });
-  expect(collection.plugins.length).toBe(4);
-  const entry = collection.get("entry-1");
-  expect(entry.hello).toBe("my friend");
-});
 
-test("should default to the original property value if value function returns falsy", async() => {
-  expect(collection.plugins.length).toBe(4);
-  await collection.mount({
-    plugins: [
-      propStore({
-        name: "oneMorePropStore",
-        prop: "hello",
-        value: () => undefined,
-        condition: () => false
-      })
-    ]
+  it("should be able to set an initial value using a function definition", async() => {
+    await collection.mount({
+      plugins: [
+        propStore({
+          prop: "hello",
+          value: () => "my" + " " + "friend",
+          condition: () => true
+        })
+      ]
+    });
+    const entry = collection.get("entry-1");
+    expect(entry.hello).toBe("my friend");
   });
-  expect(collection.plugins.length).toBe(5);
-  const entry = collection.get("entry-1");
-  expect(entry.hello).toBe("my friend");
 });

packages/drawer/tests/api.test.js

@@ -155,10 +155,15 @@ describe("register() & deregister()", () => {
   });
 
   it("should disable setting tabindex on drawer dialog", async () => {
+    // Disable tabindex before register.
     drawer.settings.setTabindex = false;
     let entry = await drawer.register("drawer-1");
     expect(entry.dialog.getAttribute("tabindex")).toBe(null);
 
+    // Deregister the entry before updating the setting and re-registering.
+    await drawer.deregister("drawer-1");
+
+    // Enable tabindex before register.
     drawer.settings.setTabindex = true;
     entry = await drawer.register("drawer-1");
     expect(entry.dialog.getAttribute("tabindex")).toBe("-1");