Skip to content

Commit

Permalink
fix: useScript failed to remove script from cache when passing `remov…
Browse files Browse the repository at this point in the history
…eOnUnmount` prop (#354)

* fix: useScript failed to remove script from cache when passing `removeOnUnmount` prop

The `removeOnUnmount` prop didn't remove the script from the cache. This created a side effect where if you try and render the same component by mounting -> unmounting -> mounting again, the `useScript` hook behaves as the script is loaded when in reality is has remove the script from the tree but failed to remove it from the cache.

* 🎨 Lint test file

* ♻️ Use a Map for the cache to avoid `delete` syntax

Prefer `cache.delete(key)` to `delete cache[key]`

* 🔖 Add changeset

---------

Co-authored-by: Julien <juliencaron@protonmail.com>
  • Loading branch information
ShanSenanayake and juliencrn authored Feb 3, 2024
1 parent 87a5141 commit 4146c39
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 4 deletions.
5 changes: 5 additions & 0 deletions .changeset/giant-shrimps-draw.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'usehooks-ts': patch
---

fix: `useScript` failed to remove script from cache when passing `removeOnUnmount` prop (#354 by @ShanSenanayake)
64 changes: 64 additions & 0 deletions packages/usehooks-ts/src/useScript/useScript.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { act, cleanup, renderHook } from '@testing-library/react'

import { useScript } from './useScript'

describe('useScript', () => {
it('should handle script loading error', () => {
const src = 'https://example.com/myscript.js'

const { result } = renderHook(() => useScript(src))

expect(result.current).toBe('loading')

act(() => {
// Simulate script error
document
.querySelector(`script[src="${src}"]`)
?.dispatchEvent(new Event('error'))
})

expect(result.current).toBe('error')
})

it('should remove script on unmount', () => {
const src = '/'

// First load the script
const { result } = renderHook(() =>
useScript(src, { removeOnUnmount: true }),
)

expect(result.current).toBe('loading')

// Make sure the document is loaded
act(() => {
document
.querySelector(`script[src="${src}"]`)
?.dispatchEvent(new Event('load'))
})

expect(result.current).toBe('ready')

// Remove the hook by unmounting and cleaning up the hook
cleanup()

// Check if the script is removed from the DOM
expect(document.querySelector(`script[src="${src}"]`)).toBeNull()

// Try loading the script again
const { result: result2 } = renderHook(() =>
useScript(src, { removeOnUnmount: true }),
)

expect(result2.current).toBe('loading')

// Make sure the document is loaded
act(() => {
document
.querySelector(`script[src="${src}"]`)
?.dispatchEvent(new Event('load'))
})

expect(result2.current).toBe('ready')
})
})
9 changes: 5 additions & 4 deletions packages/usehooks-ts/src/useScript/useScript.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export interface UseScriptOptions {
}

// Cached script statuses
const cachedScriptStatuses: Record<string, UseScriptStatus | undefined> = {}
const cachedScriptStatuses = new Map<string, UseScriptStatus | undefined>()

/**
* Gets the script element with the specified source URL.
Expand Down Expand Up @@ -54,15 +54,15 @@ export function useScript(
return 'loading'
}

return cachedScriptStatuses[src] ?? 'loading'
return cachedScriptStatuses.get(src) ?? 'loading'
})

useEffect(() => {
if (!src || options?.shouldPreventLoad) {
return
}

const cachedScriptStatus = cachedScriptStatuses[src]
const cachedScriptStatus = cachedScriptStatuses.get(src)
if (cachedScriptStatus === 'ready' || cachedScriptStatus === 'error') {
// If the script is already cached, set its status immediately
setStatus(cachedScriptStatus)
Expand Down Expand Up @@ -104,7 +104,7 @@ export function useScript(
const setStateFromEvent = (event: Event) => {
const newStatus = event.type === 'load' ? 'ready' : 'error'
setStatus(newStatus)
cachedScriptStatuses[src] = newStatus
cachedScriptStatuses.set(src, newStatus)
}

// Add event listeners
Expand All @@ -120,6 +120,7 @@ export function useScript(

if (scriptNode && options?.removeOnUnmount) {
scriptNode.remove()
cachedScriptStatuses.delete(src)
}
}
}, [src, options?.shouldPreventLoad, options?.removeOnUnmount])
Expand Down

0 comments on commit 4146c39

Please sign in to comment.