Skip to content

Commit b8317cd

Browse files
CopilotCopilot
andcommitted
Fix multiple performance and correctness issues in lazy-define.ts
- Fix race condition by using triggered Set to prevent duplicate callbacks - Add observedTargets WeakSet to avoid redundant MutationObserver calls - Disconnect MutationObserver when no pending elements remain - Add comprehensive error handling with try-catch and .catch(reportError) - Implement late registration check for already-triggered elements - Optimize scan() with early return and snapshot iteration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent c7e10be commit b8317cd

File tree

1 file changed

+102
-28
lines changed

1 file changed

+102
-28
lines changed

src/lazy-define.ts

Lines changed: 102 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
type Strategy = (tagName: string) => Promise<void>
22

3-
const dynamicElements = new Map<string, Set<() => void>>()
3+
const pending = new Map<string, Set<() => void>>()
4+
const triggered = new Set<string>()
45

56
const ready = new Promise<void>(resolve => {
67
if (document.readyState !== 'loading') {
@@ -57,54 +58,127 @@ const strategies: Record<string, Strategy> = {
5758

5859
type ElementLike = Element | Document | ShadowRoot
5960

61+
let observedTargets = new WeakSet<ElementLike>()
6062
const timers = new WeakMap<ElementLike, number>()
6163
function scan(element: ElementLike) {
62-
cancelAnimationFrame(timers.get(element) || 0)
63-
timers.set(
64-
element,
65-
requestAnimationFrame(() => {
66-
for (const tagName of dynamicElements.keys()) {
67-
const child: Element | null =
68-
element instanceof Element && element.matches(tagName) ? element : element.querySelector(tagName)
69-
if (customElements.get(tagName) || child) {
70-
const strategyName = (child?.getAttribute('data-load-on') || 'ready') as keyof typeof strategies
71-
const strategy = strategyName in strategies ? strategies[strategyName] : strategies.ready
72-
// eslint-disable-next-line github/no-then
73-
for (const cb of dynamicElements.get(tagName) || []) strategy(tagName).then(cb)
74-
dynamicElements.delete(tagName)
75-
timers.delete(element)
64+
const currentTimer = timers.get(element)
65+
if (currentTimer) cancelAnimationFrame(currentTimer)
66+
67+
const newTimer = requestAnimationFrame(() => {
68+
// FIX 7: Early return optimization
69+
if (pending.size === 0) return
70+
71+
// FIX 7: Create snapshot to iterate safely
72+
const tagList = Array.from(pending.keys())
73+
74+
for (let i = 0; i < tagList.length; i++) {
75+
const tagName = tagList[i]
76+
77+
const child: Element | null =
78+
element instanceof Element && element.matches(tagName) ? element : element.querySelector(tagName)
79+
if (customElements.get(tagName) || child) {
80+
// Only skip if already triggered AND not in pending
81+
// (If it's in pending, it means lazyDefine was called again)
82+
const shouldSkip = triggered.has(tagName) && !pending.has(tagName)
83+
if (shouldSkip) continue
84+
85+
triggered.add(tagName)
86+
87+
const callbackSet = pending.get(tagName)
88+
pending.delete(tagName)
89+
90+
const strategyName = (child?.getAttribute('data-load-on') || 'ready') as keyof typeof strategies
91+
const strategy = strategyName in strategies ? strategies[strategyName] : strategies.ready
92+
93+
// FIX 5: Wrap callback execution in try-catch and handle rejections
94+
const callbackList = Array.from(callbackSet || [])
95+
for (let j = 0; j < callbackList.length; j++) {
96+
const callback = callbackList[j]
97+
strategy(tagName)
98+
// eslint-disable-next-line github/no-then
99+
.then(() => {
100+
try {
101+
callback()
102+
} catch (err) {
103+
reportError(err)
104+
}
105+
})
106+
// eslint-disable-next-line github/no-then
107+
.catch(reportError)
76108
}
109+
110+
timers.delete(element)
77111
}
78-
})
79-
)
112+
}
113+
114+
if (pending.size === 0 && elementLoader) {
115+
elementLoader.disconnect()
116+
elementLoader = undefined
117+
observedTargets = new WeakSet<ElementLike>()
118+
}
119+
})
120+
121+
timers.set(element, newTimer)
80122
}
81123

82-
let elementLoader: MutationObserver
124+
let elementLoader: MutationObserver | undefined
83125

84126
export function lazyDefine(object: Record<string, () => void>): void
85127
export function lazyDefine(tagName: string, callback: () => void): void
86128
export function lazyDefine(tagNameOrObj: string | Record<string, () => void>, singleCallback?: () => void) {
87129
if (typeof tagNameOrObj === 'string' && singleCallback) {
88130
tagNameOrObj = {[tagNameOrObj]: singleCallback}
89131
}
132+
90133
for (const [tagName, callback] of Object.entries(tagNameOrObj)) {
91-
if (!dynamicElements.has(tagName)) dynamicElements.set(tagName, new Set<() => void>())
92-
dynamicElements.get(tagName)!.add(callback)
134+
// FIX 6: Late registration - execute immediately if already triggered
135+
// AND elements exist in DOM
136+
const wasTriggered = triggered.has(tagName)
137+
const elementsExist = wasTriggered && document.querySelector(tagName) !== null
138+
if (elementsExist) {
139+
// eslint-disable-next-line github/no-then
140+
Promise.resolve().then(() => {
141+
try {
142+
callback()
143+
} catch (err) {
144+
reportError(err)
145+
}
146+
})
147+
} else {
148+
if (!pending.has(tagName)) {
149+
pending.set(tagName, new Set<() => void>())
150+
}
151+
const callbackSet = pending.get(tagName)
152+
if (callbackSet) {
153+
callbackSet.add(callback)
154+
}
155+
}
93156
}
94157
observe(document)
95158
}
96159

97160
export function observe(target: ElementLike): void {
98-
elementLoader ||= new MutationObserver(mutations => {
99-
if (!dynamicElements.size) return
100-
for (const mutation of mutations) {
101-
for (const node of mutation.addedNodes) {
102-
if (node instanceof Element) scan(node)
161+
if (!elementLoader) {
162+
elementLoader = new MutationObserver(mutations => {
163+
if (!pending.size) return
164+
for (let i = 0; i < mutations.length; i++) {
165+
const mutation = mutations[i]
166+
const nodes = mutation.addedNodes
167+
for (let j = 0; j < nodes.length; j++) {
168+
const node = nodes[j]
169+
if (node instanceof Element) {
170+
scan(node)
171+
}
172+
}
103173
}
104-
}
105-
})
174+
})
175+
}
106176

107177
scan(target)
108178

109-
elementLoader.observe(target, {subtree: true, childList: true})
179+
// FIX 3: Check observedTargets to avoid redundant observe() calls
180+
if (!observedTargets.has(target)) {
181+
observedTargets.add(target)
182+
elementLoader.observe(target, {subtree: true, childList: true})
183+
}
110184
}

0 commit comments

Comments
 (0)