fix(useScript): variable scope bug that stops scripts being unmounted (…

…#103) * fix(useScript): variable scope bug that stops scripts being unmounted * Add changeset * Update .changeset/blue-swans-kick.md * fix(useScript): update attributes on existing script tag, fix tests to check all scripts present in head * test(useScript): test unmounting removed script element * fix(useScript): make sure script is always defined before setting attributes --------- Co-authored-by: Jovi De Croock <[email protected]>
0no-co · Jan 13, 2025 · 070924b · 070924b
1 parent 0106fef
commit 070924b
Show file tree

Hide file tree

Showing 3 changed files with 208 additions and 27 deletions.
diff --git a/.changeset/blue-swans-kick.md b/.changeset/blue-swans-kick.md
@@ -0,0 +1,5 @@
+---
+"hoofd": patch
+---
+
+Correct variable scoping in the `useEffect` of `useScript`, this prevents scripts from being wrongly unmounted
diff --git a/__tests__/useScript.test.tsx b/__tests__/useScript.test.tsx
@@ -26,9 +26,14 @@ describe('useScript', () => {
     act(() => {
       render(<MyComponent />);
     });
-    expect(document.head.innerHTML).toContain(
-      '<script type="application/javascript" crossorigin="anonymous" async="true" src="test.js"></script>'
-    );
+
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(1);
+    expect(scripts[0].type).to.equal('application/javascript');
+    expect(scripts[0].crossOrigin).to.equal('anonymous');
+    expect(scripts[0].src).toContain('test.js');
+    expect(scripts[0].getAttribute('async')).to.equal('true');
   });
 
   it('should reuse an existing tag', () => {
@@ -57,9 +62,134 @@ describe('useScript', () => {
     act(() => {
       render(<MyComponent />);
     });
-    expect(document.head.innerHTML).toContain(
-      '<script type="application/javascript" crossorigin="anonymous" async="true" src="test.js"></script>'
-    );
+
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(1);
+    expect(scripts[0].type).to.equal('application/javascript');
+    expect(scripts[0].crossOrigin).to.equal('anonymous');
+    expect(scripts[0].src).toContain('test.js');
+    expect(scripts[0].getAttribute('async')).to.equal('true');
+  });
+
+  it('should create a new tag when the existing tag with src is not found', () => {
+    const MyComponent = () => {
+      useScript({
+        crossorigin: 'anonymous',
+        type: 'application/javascript',
+        src: 'another-test.js',
+        async: true,
+      });
+      return <p>hi</p>;
+    };
+
+    const node = document.createElement('script');
+    const options = {
+      crossorigin: 'anonymous',
+      src: 'first-test.js',
+      async: true,
+    };
+    Object.keys(options).forEach(key => {
+      // @ts-ignore
+      (node as Element).setAttribute(key, options[key]);
+    });
+    document.head.appendChild(node);
+
+    act(() => {
+      render(<MyComponent />);
+    });
+
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(2);
+    expect(scripts[0].type).to.equal('');
+    expect(scripts[0].crossOrigin).to.equal('anonymous');
+    expect(scripts[0].src).toContain('first-test.js');
+    expect(scripts[0].getAttribute('async')).to.equal('true');
+
+    expect(scripts[1].type).to.equal('application/javascript');
+    expect(scripts[1].crossOrigin).to.equal('anonymous');
+    expect(scripts[1].src).toContain('another-test.js');
+    expect(scripts[1].getAttribute('async')).to.equal('true');
+
+    // cleanup only removes elements added/modified through React
+    document.head.removeChild(node);
+  });
+
+  it('should remove script on unmount', () => {
+    const MyComponent = () => {
+      useScript({
+        crossorigin: 'anonymous',
+        type: 'application/javascript',
+        src: 'test.js',
+        async: true,
+      });
+      return <p>hi</p>;
+    };
+
+    let unmount;
+
+    act(() => {
+      unmount = render(<MyComponent />).unmount;
+    });
+
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(1);
+    expect(scripts[0].type).to.equal('application/javascript');
+    expect(scripts[0].crossOrigin).to.equal('anonymous');
+    expect(scripts[0].src).toContain('test.js');
+    expect(scripts[0].getAttribute('async')).to.equal('true');
+
+    unmount();
+
+    const updatedScripts = document.head.querySelectorAll('script');
+
+    expect(updatedScripts.length).to.equal(0);
+  });
+
+  it('should remove reused script on unmount', () => {
+    const MyComponent = () => {
+      useScript({
+        crossorigin: 'anonymous',
+        type: 'application/javascript',
+        src: 'test.js',
+        async: true,
+      });
+      return <p>hi</p>;
+    };
+
+    const node = document.createElement('script');
+    const options = {
+      crossorigin: 'anonymous',
+      src: 'test.js',
+      async: true,
+    };
+    Object.keys(options).forEach(key => {
+      // @ts-ignore
+      (node as Element).setAttribute(key, options[key]);
+    });
+    document.head.appendChild(node);
+
+    let unmount;
+
+    act(() => {
+      unmount = render(<MyComponent />).unmount;
+    });
+
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(1);
+    expect(scripts[0].type).to.equal('application/javascript');
+    expect(scripts[0].crossOrigin).to.equal('anonymous');
+    expect(scripts[0].src).toContain('test.js');
+    expect(scripts[0].getAttribute('async')).to.equal('true');
+
+    unmount();
+
+    const updatedScripts = document.head.querySelectorAll('script');
+
+    expect(updatedScripts.length).to.equal(0);
   });
 
   it('should fill in the script with text', () => {
@@ -76,9 +206,11 @@ describe('useScript', () => {
       render(<MyComponent />);
     });
 
-    expect(document.head.innerHTML).toContain(
-      '<script type="application/ld+json" id="rich-text">{"key":"value"}</script>'
-    );
+    const scripts = document.head.querySelectorAll('script');
+    expect(scripts.length).to.equal(1);
+    expect(scripts[0].type).to.equal('application/ld+json');
+    expect(scripts[0].id).to.equal('rich-text');
+    expect(scripts[0].text).to.equal('{"key":"value"}');
   });
 
   it('should reuse an existing tag to fill text', () => {
@@ -106,8 +238,50 @@ describe('useScript', () => {
       render(<MyComponent />);
     });
 
-    expect(document.head.innerHTML).toContain(
-      '<script type="application/ld+json" id="rich-text">{"key":"value"}</script>'
-    );
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(1);
+    expect(scripts[0].type).to.equal('application/ld+json');
+    expect(scripts[0].id).to.equal('rich-text');
+    expect(scripts[0].text).to.equal('{"key":"value"}');
+  });
+
+  it('should create a new tag when existing tag id does not match', () => {
+    const MyComponent = () => {
+      useScript({
+        text: '{"key":"value"}',
+        type: 'application/ld+json',
+        id: 'richer-text',
+      });
+      return <p>hi</p>;
+    };
+
+    const node = document.createElement('script');
+    const options = {
+      type: 'application/ld+json',
+      id: 'empty-rich-text',
+    };
+    Object.keys(options).forEach(key => {
+      // @ts-ignore
+      (node as Element).setAttribute(key, options[key]);
+    });
+    document.head.appendChild(node);
+
+    act(() => {
+      render(<MyComponent />);
+    });
+
+    const scripts = document.head.querySelectorAll('script');
+
+    expect(scripts.length).to.equal(2);
+    expect(scripts[0].type).to.equal('application/ld+json');
+    expect(scripts[0].id).to.equal('empty-rich-text');
+    expect(scripts[0].text).to.equal('');
+    expect(scripts[1].type).to.equal('application/ld+json');
+    expect(scripts[1].id).to.equal('richer-text');
+    expect(scripts[1].text).to.equal('{"key":"value"}');
+
+    // cleanup only removes elements added/modified through React
+    document.head.removeChild(node);
   });
 });
diff --git a/src/hooks/useScript.ts b/src/hooks/useScript.ts
@@ -28,28 +28,30 @@ export const useScript = (options: ScriptOptions) => {
 
     let script: HTMLScriptElement;
 
-    if (!preExistingElements[0] && (options.src || options.id)) {
-      const script = document.createElement('script');
+    if (preExistingElements[0]) {
+      script = preExistingElements[0] as HTMLScriptElement;
+    } else {
+      script = document.createElement('script');
+    }
 
-      if (options.type) script.type = options.type;
-      if (options.module) script.type = 'module';
+    if (options.type) script.type = options.type;
+    if (options.module) script.type = 'module';
 
-      if (options.integrity)
-        script.setAttribute('integrity', options.integrity as string);
-      if (options.crossorigin)
-        script.setAttribute('crossorigin', options.crossorigin as string);
-      if (options.defer) script.setAttribute('defer', 'true');
-      else if (options.async) script.setAttribute('async', 'true');
+    if (options.integrity)
+      script.setAttribute('integrity', options.integrity as string);
+    if (options.crossorigin)
+      script.setAttribute('crossorigin', options.crossorigin as string);
+    if (options.defer) script.setAttribute('defer', 'true');
+    else if (options.async) script.setAttribute('async', 'true');
 
-      if (options.id) script.id = options.id;
+    if (options.id) script.id = options.id;
 
-      if (options.src) script.src = options.src;
+    if (options.src) script.src = options.src;
 
-      if (options.text) script.text = options.text;
+    if (options.text) script.text = options.text;
 
+    if (!preExistingElements[0]) {
       document.head.appendChild(script);
-    } else if (preExistingElements[0]) {
-      script = preExistingElements[0] as HTMLScriptElement;
     }
 
     return () => {