Skip to content

Commit 3f05783

Browse files
Copilotmattcosta7
andcommitted
Address code review feedback - improve naming and fix race conditions
Co-authored-by: mattcosta7 <8616962+mattcosta7@users.noreply.github.com>
1 parent 5d75ed1 commit 3f05783

File tree

2 files changed

+36
-37
lines changed

2 files changed

+36
-37
lines changed

src/lazy-define.ts

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,14 @@ const firstInteraction = new Promise<void>(resolve => {
2525
})
2626

2727
const visible = async (tagName: string): Promise<void> => {
28-
const makeViewportMonitor = (itemsToMonitor: Element[]) => {
29-
return new Promise<void>(signalComplete => {
30-
const viewMonitor = new IntersectionObserver(
31-
viewEvents => {
32-
for (const viewEvent of viewEvents) {
33-
if (viewEvent.isIntersecting) {
34-
signalComplete()
35-
viewMonitor.disconnect()
28+
const observeIntersection = (elements: Element[]) => {
29+
return new Promise<void>(resolve => {
30+
const observer = new IntersectionObserver(
31+
entries => {
32+
for (const entry of entries) {
33+
if (entry.isIntersecting) {
34+
resolve()
35+
observer.disconnect()
3636
return
3737
}
3838
}
@@ -46,45 +46,44 @@ const visible = async (tagName: string): Promise<void> => {
4646
threshold: 0.01
4747
}
4848
)
49-
for (const monitoredItem of itemsToMonitor) {
50-
viewMonitor.observe(monitoredItem)
49+
for (const element of elements) {
50+
observer.observe(element)
5151
}
5252
})
5353
}
5454

55-
const makeElementHunter = () => {
56-
return new Promise<Element[]>(deliverCapturedElements => {
57-
const domHunter = new MutationObserver(capturedChanges => {
58-
for (const capturedChange of capturedChanges) {
59-
const capturedNodes = Array.from(capturedChange.addedNodes)
60-
for (const capturedNode of capturedNodes) {
61-
if (!(capturedNode instanceof Element)) continue
62-
63-
const directHit = capturedNode.matches(tagName) ? capturedNode : null
64-
const nestedHit = capturedNode.querySelector(tagName)
65-
const successfulHit = directHit || nestedHit
66-
67-
if (successfulHit) {
68-
domHunter.disconnect()
69-
deliverCapturedElements(Array.from(document.querySelectorAll(tagName)))
55+
const waitForElement = () => {
56+
return new Promise<Element[]>(resolve => {
57+
const observer = new MutationObserver(mutations => {
58+
for (const mutation of mutations) {
59+
const addedNodes = Array.from(mutation.addedNodes)
60+
for (const node of addedNodes) {
61+
if (!(node instanceof Element)) continue
62+
63+
const isMatch = node.matches(tagName)
64+
const descendant = node.querySelector(tagName)
65+
66+
if (isMatch || descendant) {
67+
observer.disconnect()
68+
resolve(Array.from(document.querySelectorAll(tagName)))
7069
return
7170
}
7271
}
7372
}
7473
})
7574

76-
domHunter.observe(document.documentElement, {childList: true, subtree: true})
75+
observer.observe(document.documentElement, {childList: true, subtree: true})
7776
})
7877
}
7978

80-
const immediateFinds = Array.from(document.querySelectorAll(tagName))
79+
const existingElements = Array.from(document.querySelectorAll(tagName))
8180

82-
if (immediateFinds.length > 0) {
83-
return makeViewportMonitor(immediateFinds)
81+
if (existingElements.length > 0) {
82+
return observeIntersection(existingElements)
8483
}
8584

86-
const delayedFinds = await makeElementHunter()
87-
return makeViewportMonitor(delayedFinds)
85+
const foundElements = await waitForElement()
86+
return observeIntersection(foundElements)
8887
}
8988

9089
const strategies: Record<string, Strategy> = {

test/lazy-define.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -119,13 +119,13 @@ describe('lazyDefine', () => {
119119
it('waits for element to be added to DOM before observing', async () => {
120120
const onDefine = spy()
121121
lazyDefine('late-visible-element', onDefine)
122-
122+
123123
await animationFrame()
124124
expect(onDefine).to.be.callCount(0)
125125

126126
// Add the element later
127127
await fixture(html`<late-visible-element data-load-on="visible"></late-visible-element>`)
128-
128+
129129
await animationFrame()
130130
expect(onDefine).to.be.callCount(1)
131131
})
@@ -137,8 +137,8 @@ describe('lazyDefine', () => {
137137
lazyDefine('race-test-element', onDefine)
138138

139139
// Create multiple elements to trigger multiple scans
140-
const el1 = await fixture(html`<race-test-element></race-test-element>`)
141-
const el2 = await fixture(html`<race-test-element></race-test-element>`)
140+
await fixture(html`<race-test-element></race-test-element>`)
141+
await fixture(html`<race-test-element></race-test-element>`)
142142

143143
await animationFrame()
144144
await animationFrame()
@@ -162,7 +162,7 @@ describe('lazyDefine', () => {
162162
// Register second callback after element is already triggered
163163
lazyDefine('late-reg-element', onDefine2)
164164
await animationFrame()
165-
165+
166166
// Second callback should be executed immediately
167167
expect(onDefine2).to.be.callCount(1)
168168
})
@@ -190,7 +190,7 @@ describe('lazyDefine', () => {
190190
// Both callbacks should be called despite first one throwing
191191
expect(onDefine1).to.be.callCount(1)
192192
expect(onDefine2).to.be.callCount(1)
193-
193+
194194
// Error should have been reported
195195
expect(errors.length).to.be.greaterThan(0)
196196
} finally {

0 commit comments

Comments
 (0)