Skip to content

Commit

Permalink
Remove effect dependencies on destroy
Browse files Browse the repository at this point in the history
  • Loading branch information
oamaok committed Aug 15, 2023
1 parent 96e38e4 commit ee4243e
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 5 deletions.
15 changes: 10 additions & 5 deletions src/kaiku.ts
Original file line number Diff line number Diff line change
Expand Up @@ -330,11 +330,9 @@ const deferredUpdate = () => {

updatedDependees.add(dependeeId)

// FIXME: Technically this check shouldn't be needed, right?
const dependee = currentDependees.get(dependeeId)
if (dependee) {
updateDependee(dependee)
}
assert?.(dependee)
updateDependee(dependee)
}
}

Expand Down Expand Up @@ -403,6 +401,12 @@ const trackedExecute = <F extends (...args: any[]) => any>(

const removeDependencies = (dependee: Dependee) => {
currentDependees.delete(dependee.id_)
let keys = dependeeToKeys.get(dependee.id_)
if (keys) {
for (const key of keys) {
keyToDependees.get(key)?.delete(dependee.id_)
}
}
dependeeToKeys.delete(dependee.id_)
}

Expand Down Expand Up @@ -434,7 +438,7 @@ const createState = <T extends object>(obj: T): State<T> => {
const value = target[key as keyof T]

if (!isArray && typeof value === 'function') {
return value.bind(target)
return (value as Function).bind(target)
}

return value
Expand Down Expand Up @@ -579,6 +583,7 @@ const destroyHooks = (componentId: DependeeId) => {
const componentEffects = effects.get(componentId)
if (componentEffects) {
for (const effect of componentEffects) {
removeDependencies(effect)
effect.unsubscribe_?.()
}
}
Expand Down
45 changes: 45 additions & 0 deletions test/kaiku.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -1326,4 +1326,49 @@ describe('kaiku', () => {
await nextTick()
expect(inputElem.value).toBe('foobar')
})

it('should not run effects of unmounted components', async () => {
const effectCounter = jest.fn()
const unsubCounter = jest.fn()
const state = createState({
isCompMounted: true,
counter: 0,
})

const Comp = () => {
useEffect(() => {
effectCounter(state.counter)
return unsubCounter
})

return <div />
}

const App = () => {
return (
<div>
{state.isCompMounted ? <Comp /> : null}
</div>
)
}

render(<App />, rootNode)
expect(effectCounter).toHaveBeenCalledTimes(1)
expect(unsubCounter).toHaveBeenCalledTimes(0)

state.counter++
await nextTick()
expect(effectCounter).toHaveBeenCalledTimes(2)
expect(unsubCounter).toHaveBeenCalledTimes(1)

state.isCompMounted = false
await nextTick()
expect(effectCounter).toHaveBeenCalledTimes(2)
expect(unsubCounter).toHaveBeenCalledTimes(2)

state.counter++
await nextTick()
expect(effectCounter).toHaveBeenCalledTimes(2)
expect(unsubCounter).toHaveBeenCalledTimes(2)
})
})

0 comments on commit ee4243e

Please sign in to comment.