diff --git a/package-lock.json b/package-lock.json index 457970e..940cefa 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1903,6 +1903,7 @@ "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-5.36.1.tgz", "integrity": "sha512-/IsgNGOkBi7CuDfUbwt1eOqUXF9WGVBW9dwEe1pi+L32XrTsZIgmDFIi2RxjzsvB/8i+MIf5JIoTEH8LOZ368A==", "dev": true, + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "5.36.1", "@typescript-eslint/types": "5.36.1", @@ -2696,6 +2697,7 @@ "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.8.0.tgz", "integrity": "sha512-QOxyigPVrpZ2GXT+PFyZTl6TtOFc5egxHIP9IlQ+RbupQuX4RkT/Bee4/kQuC02Xkzg84JcT7oLYtDIQxp+v7w==", "dev": true, + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -3990,7 +3992,8 @@ "resolved": "https://registry.npmjs.org/devtools-protocol/-/devtools-protocol-0.0.1534754.tgz", "integrity": "sha512-26T91cV5dbOYnXdJi5qQHoTtUoNEqwkHcAyu/IKtjIAxiEqPMrDiRkDOPWVsGfNZGmlQVHQbZRSjD8sxagWVsQ==", "dev": true, - "license": "BSD-3-Clause" + "license": "BSD-3-Clause", + "peer": true }, "node_modules/diff": { "version": "5.0.0", @@ -4641,6 +4644,7 @@ "integrity": "sha512-pBG/XOn0MsJcKcTRLr27S5HpzQo4kLr+HjLQIyK4EiCsijDl/TB+h5uEuJU6bQ8Edvwz1XWOjpaP2qgnXGpTcA==", "deprecated": "This version is no longer supported. Please see https://eslint.org/version-support for other options.", "dev": true, + "peer": true, "dependencies": { "@eslint/eslintrc": "^1.3.1", "@humanwhocodes/config-array": "^0.10.4", @@ -8395,6 +8399,7 @@ "resolved": "https://registry.npmjs.org/prettier/-/prettier-2.6.2.tgz", "integrity": "sha512-PkUpF+qoXTqhOeWL9fu7As8LXsIUZ1WYaJiY/a7McAQzxjk82OF0tibkFXVCDImZtWxbvojFjerkiLb0/q8mew==", "dev": true, + "peer": true, "bin": { "prettier": "bin-prettier.js" }, @@ -8840,6 +8845,7 @@ "integrity": "sha512-PggGy4dhwx5qaW+CKBilA/98Ql9keyfnb7lh4SR6shQ91QQQi1ORJ1v4UinkdP2i87OBs9AQFooQylcrrRfIcg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@types/estree": "1.0.8" }, @@ -9194,6 +9200,7 @@ "resolved": "https://registry.npmjs.org/size-limit/-/size-limit-8.0.1.tgz", "integrity": "sha512-VHrozqkQTYfcv1OlZIRIL0x6f+xhZ3TT+RTXC5AvKn/yA+3PIWERrKWqHMJPD7G/Vi0SuBtWAn3IvCGx2/UB1g==", "dev": true, + "peer": true, "dependencies": { "bytes-iec": "^3.1.1", "chokidar": "^3.5.3", @@ -9811,6 +9818,7 @@ "resolved": "https://registry.npmjs.org/typescript/-/typescript-4.8.2.tgz", "integrity": "sha512-C0I1UsrrDHo2fYI5oaCGbSejwX4ch+9Y5jTQELvovfmFkK3HHSZJB8MSJcWLmCUBzQBchCrZ9rMRV6GuNrvGtw==", "dev": true, + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" diff --git a/src/lazy-define.ts b/src/lazy-define.ts index 016b886..9918029 100644 --- a/src/lazy-define.ts +++ b/src/lazy-define.ts @@ -1,6 +1,7 @@ type Strategy = (tagName: string) => Promise -const dynamicElements = new Map void>>() +const pending = new Map void>>() +const triggered = new Set() const ready = new Promise(resolve => { if (document.readyState !== 'loading') { @@ -23,31 +24,67 @@ const firstInteraction = new Promise(resolve => { document.addEventListener('pointerdown', handler, listenerOptions) }) -const visible = (tagName: string): Promise => - new Promise(resolve => { - const observer = new IntersectionObserver( - entries => { - for (const entry of entries) { - if (entry.isIntersecting) { - resolve() - observer.disconnect() - return +const visible = async (tagName: string): Promise => { + const observeIntersection = (elements: Element[]) => { + return new Promise(resolve => { + const observer = new IntersectionObserver( + entries => { + for (const entry of entries) { + if (entry.isIntersecting) { + resolve() + observer.disconnect() + return + } } + }, + { + // Currently the threshold is set to 256px from the bottom of the viewport + // with a threshold of 0.1. This means the element will not load until about + // 2 keyboard-down-arrow presses away from being visible in the viewport, + // giving us some time to fetch it before the contents are made visible + rootMargin: '0px 0px 256px 0px', + threshold: 0.01 } - }, - { - // Currently the threshold is set to 256px from the bottom of the viewport - // with a threshold of 0.1. This means the element will not load until about - // 2 keyboard-down-arrow presses away from being visible in the viewport, - // giving us some time to fetch it before the contents are made visible - rootMargin: '0px 0px 256px 0px', - threshold: 0.01 + ) + for (const element of elements) { + observer.observe(element) } - ) - for (const el of document.querySelectorAll(tagName)) { - observer.observe(el) - } - }) + }) + } + + const waitForElement = () => { + return new Promise(resolve => { + const observer = new MutationObserver(mutations => { + for (const mutation of mutations) { + const addedNodes = Array.from(mutation.addedNodes) + for (const node of addedNodes) { + if (!(node instanceof Element)) continue + + const isMatch = node.matches(tagName) + const descendant = node.querySelector(tagName) + + if (isMatch || descendant) { + observer.disconnect() + resolve(Array.from(document.querySelectorAll(tagName))) + return + } + } + } + }) + + observer.observe(document.documentElement, {childList: true, subtree: true}) + }) + } + + const existingElements = Array.from(document.querySelectorAll(tagName)) + + if (existingElements.length > 0) { + return observeIntersection(existingElements) + } + + const foundElements = await waitForElement() + return observeIntersection(foundElements) +} const strategies: Record = { ready: () => ready, @@ -57,29 +94,71 @@ const strategies: Record = { type ElementLike = Element | Document | ShadowRoot +const observedTargets = new WeakSet() const timers = new WeakMap() + +function cleanupObserver() { + if (pending.size === 0 && elementLoader) { + elementLoader.disconnect() + elementLoader = undefined + } +} + function scan(element: ElementLike) { - cancelAnimationFrame(timers.get(element) || 0) - timers.set( - element, - requestAnimationFrame(() => { - for (const tagName of dynamicElements.keys()) { - const child: Element | null = - element instanceof Element && element.matches(tagName) ? element : element.querySelector(tagName) - if (customElements.get(tagName) || child) { - const strategyName = (child?.getAttribute('data-load-on') || 'ready') as keyof typeof strategies - const strategy = strategyName in strategies ? strategies[strategyName] : strategies.ready - // eslint-disable-next-line github/no-then - for (const cb of dynamicElements.get(tagName) || []) strategy(tagName).then(cb) - dynamicElements.delete(tagName) - timers.delete(element) + const currentTimer = timers.get(element) + if (currentTimer) cancelAnimationFrame(currentTimer) + + const newTimer = requestAnimationFrame(() => { + // FIX 7: Early return optimization + if (pending.size === 0) return + + // FIX 7: Create snapshot to prevent modification-during-iteration issues + // (concurrent scans may delete tags from pending) + const tagList = Array.from(pending.keys()) + + for (const tagName of tagList) { + const child: Element | null = + element instanceof Element && element.matches(tagName) ? element : element.querySelector(tagName) + if (customElements.get(tagName) || child) { + // Skip if already processed and not re-registered + if (triggered.has(tagName) && !pending.has(tagName)) continue + + triggered.add(tagName) + + const callbackSet = pending.get(tagName) + pending.delete(tagName) + + const strategyName = (child?.getAttribute('data-load-on') || 'ready') as keyof typeof strategies + const strategy = strategyName in strategies ? strategies[strategyName] : strategies.ready + + // FIX 5: Wrap callback execution in try-catch and handle rejections + const callbackList = Array.from(callbackSet || []) + for (const callback of callbackList) { + strategy(tagName) + // eslint-disable-next-line github/no-then + .then(() => { + try { + callback() + } catch (err) { + reportError(err) + } + }) + // eslint-disable-next-line github/no-then + .catch(reportError) } + + timers.delete(element) } - }) - ) + } + + // FIX 4: Disconnect observer when all pending tags are processed + cleanupObserver() + }) + + timers.set(element, newTimer) } -let elementLoader: MutationObserver +let elementLoader: MutationObserver | undefined export function lazyDefine(object: Record void>): void export function lazyDefine(tagName: string, callback: () => void): void @@ -87,24 +166,49 @@ export function lazyDefine(tagNameOrObj: string | Record void>, si if (typeof tagNameOrObj === 'string' && singleCallback) { tagNameOrObj = {[tagNameOrObj]: singleCallback} } + for (const [tagName, callback] of Object.entries(tagNameOrObj)) { - if (!dynamicElements.has(tagName)) dynamicElements.set(tagName, new Set<() => void>()) - dynamicElements.get(tagName)!.add(callback) + // FIX 6: Late registration - execute immediately if already triggered + // Check both triggered state and element existence to avoid executing for removed elements + if (triggered.has(tagName) && document.querySelector(tagName)) { + // eslint-disable-next-line github/no-then + Promise.resolve().then(() => { + try { + callback() + } catch (err) { + reportError(err) + } + }) + } else { + if (!pending.has(tagName)) { + pending.set(tagName, new Set<() => void>()) + } + pending.get(tagName)!.add(callback) + } } observe(document) } export function observe(target: ElementLike): void { - elementLoader ||= new MutationObserver(mutations => { - if (!dynamicElements.size) return - for (const mutation of mutations) { - for (const node of mutation.addedNodes) { - if (node instanceof Element) scan(node) + if (!elementLoader) { + elementLoader = new MutationObserver(mutations => { + if (!pending.size) return + for (const mutation of mutations) { + const nodes = mutation.addedNodes + for (const node of nodes) { + if (node instanceof Element) { + scan(node) + } + } } - } - }) + }) + } scan(target) - elementLoader.observe(target, {subtree: true, childList: true}) + // FIX 3: Check observedTargets to avoid redundant observe() calls + if (!observedTargets.has(target)) { + observedTargets.add(target) + elementLoader.observe(target, {subtree: true, childList: true}) + } } diff --git a/test/lazy-define.ts b/test/lazy-define.ts index b2517d6..23a2315 100644 --- a/test/lazy-define.ts +++ b/test/lazy-define.ts @@ -1,5 +1,5 @@ import {expect, fixture, html} from '@open-wc/testing' -import {spy} from 'sinon' +import {spy, stub} from 'sinon' import {lazyDefine, observe} from '../src/lazy-define.js' const animationFrame = () => new Promise(resolve => requestAnimationFrame(resolve)) @@ -116,4 +116,92 @@ describe('lazyDefine', () => { expect(onDefine).to.be.callCount(1) }) }) + + describe('race condition prevention', () => { + it('does not fire callbacks multiple times from concurrent scans', async () => { + const onDefine = spy() + lazyDefine('race-test-element', onDefine) + + // Create multiple elements to trigger multiple scans + await fixture(html``) + await fixture(html``) + + await animationFrame() + await animationFrame() + + // Should only be called once despite multiple elements triggering scans + expect(onDefine).to.be.callCount(1) + }) + }) + + describe('late registration', () => { + it('executes callback immediately for already-triggered tags', async () => { + const onDefine1 = spy() + const onDefine2 = spy() + + // Register and trigger first callback + lazyDefine('late-reg-element', onDefine1) + await fixture(html``) + await animationFrame() + expect(onDefine1).to.be.callCount(1) + + // Register second callback after element is already triggered + lazyDefine('late-reg-element', onDefine2) + await animationFrame() + + // Second callback should be executed immediately + expect(onDefine2).to.be.callCount(1) + }) + }) + + describe('error handling', () => { + it('handles callback errors without breaking other callbacks', async () => { + const onDefine1 = spy(() => { + throw new Error('Test error') + }) + const onDefine2 = spy() + + const errors: unknown[] = [] + const reportErrorStub = stub(globalThis, 'reportError').callsFake((err: unknown) => errors.push(err)) + + try { + lazyDefine('error-test-element', onDefine1) + lazyDefine('error-test-element', onDefine2) + + await fixture(html``) + await animationFrame() + + // Both callbacks should be called despite first one throwing + expect(onDefine1).to.be.callCount(1) + expect(onDefine2).to.be.callCount(1) + + // Error should have been reported + expect(errors.length).to.be.greaterThan(0) + } finally { + reportErrorStub.restore() + } + }) + }) + + describe('redundant observe calls', () => { + it('does not observe the same target multiple times', async () => { + const onDefine = spy() + const el = await fixture(html`
`) + const shadowRoot = el.attachShadow({mode: 'open'}) + + // Observe the same shadow root multiple times + observe(shadowRoot) + observe(shadowRoot) + observe(shadowRoot) + + lazyDefine('redundant-test-element', onDefine) + // eslint-disable-next-line github/unescaped-html-literal + shadowRoot.innerHTML = '' + + await animationFrame() + + // Should still only be called once + expect(onDefine).to.be.callCount(1) + }) + }) })