fix(scripts): prevent scope disposal from aborting unrelated trigger in useScript (#660)

Author: cernymatej

packages/unhead/src/scripts/types.ts

@@ -95,7 +95,7 @@ export interface ScriptInstance<T extends BaseScriptApi> {
   /**
    * @internal
    */
-  _triggerAbortPromise?: Promise<void>
+  _triggerAbortControllers?: Set<AbortController>
   /**
    * @internal
    */

packages/unhead/src/scripts/useScript.ts

@@ -116,8 +116,9 @@ export function useScript<T extends Record<symbol | string, any> = Record<symbol
     status: 'awaitingLoad',
 
     remove() {
-      // cancel any pending triggers as we've started loading
-      script._triggerAbortController?.abort()
+      // cancel all pending triggers
+      script._triggerAbortControllers?.forEach(ac => ac.abort())
+      script._triggerAbortControllers?.clear()
       script._triggerPromises = [] // clear any pending promises
       script._warmupEl?.dispose()
       if (script.entry) {
@@ -154,8 +155,9 @@ export function useScript<T extends Record<symbol | string, any> = Record<symbol
       return script._warmupEl
     },
     load(cb?: () => void | Promise<void>) {
-      // cancel any pending triggers as we've started loading
-      script._triggerAbortController?.abort()
+      // cancel all pending triggers as we've started loading
+      script._triggerAbortControllers?.forEach(ac => ac.abort())
+      script._triggerAbortControllers?.clear()
       script._triggerPromises = [] // clear any pending promises
       if (!script.entry) {
         syncStatus('loading')
@@ -195,19 +197,23 @@ export function useScript<T extends Record<symbol | string, any> = Record<symbol
         if (head.ssr) {
           return
         }
-        if (!script._triggerAbortController) {
-          script._triggerAbortController = new AbortController()
-          script._triggerAbortPromise = new Promise<void>((resolve) => {
-            script._triggerAbortController!.signal.addEventListener('abort', () => {
-              script._triggerAbortController = null
-              resolve()
-            })
+        // each trigger gets its own abort controller so that disposing one scope
+        // does not cancel triggers from other scopes
+        const abortController = new AbortController()
+        script._triggerAbortControllers = script._triggerAbortControllers || new Set()
+        script._triggerAbortControllers.add(abortController)
+        const abortPromise = new Promise<void>((resolve) => {
+          abortController.signal.addEventListener('abort', () => {
+            script._triggerAbortControllers?.delete(abortController)
+            resolve()
           })
-        }
+        })
+        // store the latest controller for external access
+        script._triggerAbortController = abortController
         script._triggerPromises = script._triggerPromises || []
         const idx = script._triggerPromises.push(Promise.race([
           trigger.then(v => typeof v === 'undefined' || v ? script.load : undefined),
-          script._triggerAbortPromise,
+          abortPromise,
         ])
           // OK
           .catch(() => {})

packages/vue/src/scripts/useScript.ts

@@ -61,9 +61,11 @@ function registerVueScopeHandlers<T extends Record<symbol | string, any> = Recor
   // if we have a scope we should make these callbacks reactive
   script.onLoaded = (cb: (instance: T) => void | Promise<void>) => _registerCb('loaded', cb)
   script.onError = (cb: (err?: Error) => void | Promise<void>) => _registerCb('error', cb)
+  // capture the controller at registration time so this scope only aborts
+  // the controller it was associated with, not a newer one created by a later scope
+  const triggerAbortController = script._triggerAbortController
   onScopeDispose(() => {
-    // stop any trigger promises
-    script._triggerAbortController?.abort()
+    triggerAbortController?.abort()
   })
 }
 

packages/vue/test/unit/useScript.test.ts

@@ -1,8 +1,9 @@
+// @vitest-environment jsdom
 import { createHead } from '@unhead/vue/client'
 import { renderSSRHead } from '@unhead/vue/server'
 
 import { describe, it } from 'vitest'
-import { ref, watch } from 'vue'
+import { createApp, h, ref, watch } from 'vue'
 import { useDom } from '../../../unhead/test/util'
 import { createHeadCore } from '../../src'
 import { useScript } from '../../src/scripts/useScript'
@@ -201,4 +202,59 @@ describe('vue e2e scripts', () => {
     `)
     script.remove()
   })
+
+  it('setupTriggerHandler race condition: old scope disposal should not abort new scope trigger', async () => {
+    const dom = useDom()
+    const head = createHead({
+      document: dom.window.document,
+    })
+
+    // simulate a first page visit: component uses useScript with a trigger that never resolves
+    // and the user navigates away before interacting
+    const el1 = dom.window.document.createElement('div')
+    const app1 = createApp({
+      setup() {
+        useScript({
+          src: '//race-condition-script.js',
+        }, {
+          trigger: new Promise(() => {}), // never resolves
+          head,
+        })
+        return () => h('div')
+      },
+    })
+    app1.mount(el1)
+
+    const script = (head as any)._scripts['//race-condition-script.js']
+    expect(script.status).toBe('awaitingLoad')
+    expect(script._triggerAbortController).toBeTruthy()
+
+    const { resolve: resolveTrigger2, promise: trigger2 } = Promise.withResolvers<void>()
+
+    // simulate client-side navigation: the new page mounts BEFORE the old page unmounts
+    const el2 = dom.window.document.createElement('div')
+    const app2 = createApp({
+      setup() {
+        useScript({
+          src: '//race-condition-script.js',
+        }, {
+          trigger: trigger2,
+          head,
+        })
+        return () => h('div')
+      },
+    })
+    app2.mount(el2)
+
+    expect(script.status).toBe('awaitingLoad')
+
+    app1.unmount()
+
+    // the user interacts on the new page, resolving `trigger2`
+    resolveTrigger2()
+    await new Promise<void>(resolve => setTimeout(resolve, 0))
+
+    // as `trigger2` is now resolved, we expect `script.load()` to have been called
+    expect(script.status).toBe('loading')
+  })
 })