From 33d2623c2637ea160d0a10dbabc09754d3679fba Mon Sep 17 00:00:00 2001 From: George Treviranus Date: Sun, 16 May 2021 10:46:04 -0700 Subject: [PATCH] Revert "updates base class to set firstRender to false immediately to prevent double mounts" This reverts commit dd0ff865258bc64a26fd2a12f47e53742a3afd81. --- src/__tests__/fixtures/accessor-fixture.js | 2 +- src/__tests__/fixtures/basic-fixture.js | 2 +- src/__tests__/fixtures/lifecycle-fixture.js | 2 +- src/__tests__/upgraded-element.spec.js | 66 ++++++++++++--------- src/internal.js | 4 +- src/upgraded-element.js | 14 ++--- 6 files changed, 49 insertions(+), 41 deletions(-) diff --git a/src/__tests__/fixtures/accessor-fixture.js b/src/__tests__/fixtures/accessor-fixture.js index 9a3b3da..a7744ef 100644 --- a/src/__tests__/fixtures/accessor-fixture.js +++ b/src/__tests__/fixtures/accessor-fixture.js @@ -5,7 +5,7 @@ import { UpgradedElement, register } from "../.." * can't unregister custom elements from the DOM, even between tests. * @param {string} id */ -export function createAccessorFixture(id) { +export function accessorFixture(id) { class TestElement extends UpgradedElement { constructor() { super() diff --git a/src/__tests__/fixtures/basic-fixture.js b/src/__tests__/fixtures/basic-fixture.js index e899726..c16e174 100644 --- a/src/__tests__/fixtures/basic-fixture.js +++ b/src/__tests__/fixtures/basic-fixture.js @@ -6,7 +6,7 @@ import { UpgradedElement, register } from "../.." * @param {string} id * @param {{content: string, properties: Object, styles: string}} */ -export function createBasicFixture(id, options = {}) { +export function basicFixture(id, options = {}) { class TestElement extends UpgradedElement { constructor() { super() diff --git a/src/__tests__/fixtures/lifecycle-fixture.js b/src/__tests__/fixtures/lifecycle-fixture.js index 8e88315..012469d 100644 --- a/src/__tests__/fixtures/lifecycle-fixture.js +++ b/src/__tests__/fixtures/lifecycle-fixture.js @@ -5,7 +5,7 @@ import { UpgradedElement, register } from "../.." * can't unregister custom elements from the DOM, even between tests. * @param {string} id */ -export function createLifecycleFixture(id, wait = false) { +export function lifecycleFixture(id, wait = false) { class TestElement extends UpgradedElement { constructor() { super() diff --git a/src/__tests__/upgraded-element.spec.js b/src/__tests__/upgraded-element.spec.js index 500639a..5ece810 100644 --- a/src/__tests__/upgraded-element.spec.js +++ b/src/__tests__/upgraded-element.spec.js @@ -1,24 +1,25 @@ -import { createBasicFixture } from "./fixtures/basic-fixture" -import { createAccessorFixture } from "./fixtures/accessor-fixture" -import { createLifecycleFixture } from "./fixtures/lifecycle-fixture" +import { basicFixture } from "./fixtures/basic-fixture" +import { accessorFixture } from "./fixtures/accessor-fixture" +import { lifecycleFixture } from "./fixtures/lifecycle-fixture" import { getElement } from "./fixtures/get-element" import * as external from "../external" window.requestAnimationFrame = jest.fn().mockImplementation((fn) => fn()) +window.cancelAnimationFrame = jest.fn().mockImplementation((fn) => fn()) describe("UpgradedElement", () => { afterEach(() => (document.innerHTML = "")) it("upgrades the element", () => { // Given - createBasicFixture("upgraded") + basicFixture("upgraded") // Then expect(getElement("upgraded").hasAttribute("element-id")).toBe(true) }) it("creates a shadow root", () => { // Given - createBasicFixture("creates-shadow") + basicFixture("creates-shadow") // Then expect(getElement("creates-shadow").shadowRoot).not.toBeNull() }) @@ -26,7 +27,7 @@ describe("UpgradedElement", () => { it("renders styles to shadow root", () => { // Given const styles = "div { display: block; }" - createBasicFixture("styles", { styles }) + basicFixture("styles", { styles }) // Then expect( getElement("styles").shadowRoot.querySelector("style") @@ -39,7 +40,7 @@ describe("UpgradedElement", () => { it("assigns slots, if given", () => { // Given const slotName = "main" - createBasicFixture("slotted", { + basicFixture("slotted", { slotName, content: ``, }) @@ -55,7 +56,7 @@ describe("UpgradedElement", () => { const properties = { testProp1: { default: "foo" }, } - createBasicFixture("props", { properties }) + basicFixture("props", { properties }) // Then expect(getElement("props").testProp1).toEqual( properties.testProp1.default @@ -68,7 +69,7 @@ describe("UpgradedElement", () => { testProp1: { default: "foo" }, } const nextValue = "bar" - createBasicFixture("val-change", { properties }) + basicFixture("val-change", { properties }) // When getElement("val-change").testProp1 = nextValue // Then @@ -80,7 +81,7 @@ describe("UpgradedElement", () => { it("doesn't upgrade properties if accessors already exist", () => { // Given - createAccessorFixture("no-upgrade") + accessorFixture("no-upgrade") // Then expect(getElement("no-upgrade").count).toEqual(1) }) @@ -95,7 +96,7 @@ describe("UpgradedElement", () => { safe: true, }, } - createBasicFixture("safe-upgrade", { properties }) + basicFixture("safe-upgrade", { properties }) // Then const nextValue = "<span>unsafe</span>" expect(getElement("safe-upgrade").safeString).toEqual(nextValue) @@ -110,7 +111,7 @@ describe("UpgradedElement", () => { safe: true, }, } - createBasicFixture("safe-change", { properties }) + basicFixture("safe-change", { properties }) // When getElement("safe-change").safeString = "&hello" // Then @@ -125,7 +126,7 @@ describe("UpgradedElement", () => { const properties = { reflectedProp: { reflected: true }, } - createBasicFixture("reflect-one", { properties }) + basicFixture("reflect-one", { properties }) // Then const element = getElement("reflect-one") expect(element.hasAttribute("reflected-prop")).toBe(true) @@ -137,7 +138,7 @@ describe("UpgradedElement", () => { const properties = { reflectedProp: { default: "foo", reflected: true }, } - createBasicFixture("reflect-two", { properties }) + basicFixture("reflect-two", { properties }) // Then const element = getElement("reflect-two") expect(element.hasAttribute("reflected-prop")).toBe(true) @@ -149,7 +150,7 @@ describe("UpgradedElement", () => { const properties = { reflectedProp: { default: "foo", reflected: true }, } - createBasicFixture("reflect-three", { properties }) + basicFixture("reflect-three", { properties }) const element = getElement("reflect-three") element.reflectedProp = "bar" // Then @@ -161,7 +162,7 @@ describe("UpgradedElement", () => { const properties = { reflectedProp: { default: "foo", reflected: true }, } - createBasicFixture("reflect-four", { properties }) + basicFixture("reflect-four", { properties }) const element = getElement("reflect-four") element.reflectedProp = undefined // Then @@ -184,7 +185,7 @@ describe("UpgradedElement", () => { default: "foo", }, } - createBasicFixture("upgrade-type-warn", { properties }) + basicFixture("upgrade-type-warn", { properties }) // Then expect(console.warn).toBeCalledWith(warningMessage) }) @@ -197,7 +198,7 @@ describe("UpgradedElement", () => { default: true, }, } - createBasicFixture("change-type-warn", { properties }) + basicFixture("change-type-warn", { properties }) // When getElement("change-type-warn").testProp1 = "foo" // Then @@ -211,7 +212,7 @@ describe("UpgradedElement", () => { describe("lifecycle methods", () => { it("calls elementPropertyChanged", () => { - const Cls = createLifecycleFixture("prop-changed") + const Cls = lifecycleFixture("prop-changed") Cls.prototype[external.elementPropertyChanged] = jest.fn() getElement("prop-changed").testProp = true expect(Cls.prototype[external.elementPropertyChanged]).toBeCalledWith( @@ -222,7 +223,7 @@ describe("UpgradedElement", () => { }) it("calls elementAttributeChanged", () => { - const Cls = createLifecycleFixture("attr-changed") + const Cls = lifecycleFixture("attr-changed") Cls.prototype[external.elementAttributeChanged] = jest.fn() getElement("attr-changed").testProp = true expect(Cls.prototype[external.elementAttributeChanged]).toBeCalledWith( @@ -233,45 +234,54 @@ describe("UpgradedElement", () => { }) it("calls elementDidUpdate", () => { - const Cls = createLifecycleFixture("update") + const Cls = lifecycleFixture("update") Cls.prototype[external.elementDidUpdate] = jest.fn() getElement("update").testProp = true expect(Cls.prototype[external.elementDidUpdate]).toBeCalled() }) - it("calls elementDidUpdate if property updates in elementDidMount", async () => { - const [init, Cls] = createLifecycleFixture("mount-update", true) + // whyyyyy + // eslint-disable-next-line jest/no-disabled-tests + it.skip("calls elementDidUpdate if property updates in elementDidMount", async () => { + const [init, Cls] = lifecycleFixture("mount-update", true) + Cls.prototype[external.elementDidUpdate] = jest.fn() Cls.prototype[external.elementDidMount] = () => { getElement("mount-update").testProp = true } - Cls.prototype[external.elementDidUpdate] = jest.fn() + // Cls.prototype[external.elementDidMount] = () => { + // console.log("Mounted!") + // getElement("mount-update").testProp = true + // } + // Cls.prototype[external.elementDidUpdate] = () => { + // console.log("Updated!") + // } init() expect(Cls.prototype[external.elementDidUpdate]).toBeCalled() }) it("calls elementDidConnect", () => { - const [init, Cls] = createLifecycleFixture("connect", true) + const [init, Cls] = lifecycleFixture("connect", true) Cls.prototype[external.elementDidConnect] = jest.fn() init() expect(Cls.prototype[external.elementDidConnect]).toBeCalled() }) it("calls elementDidMount", () => { - const [init, Cls] = createLifecycleFixture("mount", true) + const [init, Cls] = lifecycleFixture("mount", true) Cls.prototype[external.elementDidMount] = jest.fn() init() expect(Cls.prototype[external.elementDidMount]).toBeCalled() }) it("calls elementWillUnmount", () => { - const Cls = createLifecycleFixture("unmount") + const Cls = lifecycleFixture("unmount") Cls.prototype[external.elementWillUnmount] = jest.fn() document.body.removeChild(getElement("unmount")) expect(Cls.prototype[external.elementWillUnmount]).toBeCalled() }) it("recalls elementDidMount if the component is disconnected and then reconnected", async () => { - const [init, Cls] = createLifecycleFixture("remount", true) + const [init, Cls] = lifecycleFixture("remount", true) Cls.prototype[external.elementDidMount] = jest.fn() init() const fixture = getElement("remount") diff --git a/src/internal.js b/src/internal.js index 5028f20..0e1ba4f 100644 --- a/src/internal.js +++ b/src/internal.js @@ -16,6 +16,6 @@ export const runLifecycle = Symbol("#runLifecycle") export const performUpgrade = Symbol("#performUpgrade") export const upgradeProperty = Symbol("#upgradeProperty") export const renderStyles = Symbol("#renderStyles") -export const setInitialRenderState = Symbol("#setInitialRenderState") -export const setNextRenderState = Symbol("#setNextRenderState") +export const getInitialRenderState = Symbol("#getInitialRenderState") +export const getNextRenderState = Symbol("#getNextRenderState") export const renderDOM = Symbol("#renderDOM") diff --git a/src/upgraded-element.js b/src/upgraded-element.js index 4343317..cbe4104 100644 --- a/src/upgraded-element.js +++ b/src/upgraded-element.js @@ -220,17 +220,18 @@ export class UpgradedElement extends HTMLElement { * Create a virtual DOM from the external `render` method and patch * it into the shadow root. Triggers `elementDidMount`, if defined. */ - [internal.setInitialRenderState]() { + [internal.getInitialRenderState]() { this[internal.vDOM] = this[internal.getVDOM]() render(this[internal.vDOM], this[internal.shadowRoot]) this[internal.runLifecycle](external.elementDidMount) + this[internal.isFirstRender] = false } /** * All renders after initial render: * Create a new vdom and patch the existing one. */ - [internal.setNextRenderState]() { + [internal.getNextRenderState]() { let nextVDOM = this[internal.getVDOM]() patch(nextVDOM, this[internal.vDOM]) this[internal.runLifecycle](external.elementDidUpdate) @@ -241,11 +242,8 @@ export class UpgradedElement extends HTMLElement { * Runs either a new render or diffs the existing virtual DOM to a new one. */ [internal.renderDOM]() { - if (this[internal.isFirstRender]) { - this[internal.isFirstRender] = false - this[internal.setInitialRenderState]() - } else { - this[internal.setNextRenderState]() - } + return this[internal.isFirstRender] + ? this[internal.getInitialRenderState]() + : this[internal.getNextRenderState]() } }