Skip to content

Commit

Permalink
Fix useLoadScript to avoid infinite rerenders (#1775)
Browse files Browse the repository at this point in the history
* Refactor: simplify code

* Fix: infinite re-rendering when using second param of useLoadScript

* Changesets

* Perf: stop serializing options

* Test: ensure script finishes loading without infinite rerenders
  • Loading branch information
frandiox committed Feb 21, 2024
1 parent 06d9fd9 commit 409e1bc
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 17 deletions.
5 changes: 5 additions & 0 deletions .changeset/sweet-mayflies-type.md
@@ -0,0 +1,5 @@
---
'@shopify/hydrogen-react': patch
---

Fix `useLoadScript` to avoid infinite re-renders when using its second parameter.
16 changes: 15 additions & 1 deletion packages/hydrogen-react/src/load-script.test.tsx
@@ -1,5 +1,5 @@
import {vi, afterEach, describe, expect, it} from 'vitest';
import {renderHook} from '@testing-library/react';
import {renderHook, act, findByTestId} from '@testing-library/react';
import {useLoadScript} from './load-script.js';

let html: HTMLHtmlElement;
Expand Down Expand Up @@ -82,4 +82,18 @@ describe(`useLoadScript`, () => {
const scripts = html.querySelectorAll('body script');
expect(scripts.length).toEqual(2);
});

it('finishes loading a script', async () => {
const hookResult = renderHook(() =>
useLoadScript('test6.js', {attributes: {'data-testid': 'test'}}),
);

await act(async () => {
const script = await findByTestId<HTMLScriptElement>(html, 'test');
expect(hookResult.result.current).toBe('loading');
script.onload?.({} as Event);
});

expect(hookResult.result.current).toBe('done');
});
});
26 changes: 10 additions & 16 deletions packages/hydrogen-react/src/load-script.tsx
Expand Up @@ -62,23 +62,17 @@ export function useLoadScript(
options?: LoadScriptParams[1],
): ScriptState {
const [status, setStatus] = useState<ScriptState>('loading');
const stringifiedOptions = JSON.stringify(options);

useEffect(() => {
async function loadScriptWrapper(): Promise<void> {
try {
setStatus('loading');
await loadScript(url, options);
setStatus('done');
} catch (error) {
setStatus('error');
}
}

loadScriptWrapper().catch(() => {
setStatus('error');
});
}, [url, stringifiedOptions, options]);
useEffect(
() => {
loadScript(url, options)
.then(() => setStatus('done'))
.catch(() => setStatus('error'));
},
// Ignore options changes since it won't trigger a new load.
// eslint-disable-next-line react-hooks/exhaustive-deps
[url],
);

return status;
}
Expand Down

0 comments on commit 409e1bc

Please sign in to comment.