hupe1980 / react-script-hook

React hook to dynamically load an external script and know when its loaded
MIT License
127 stars 21 forks source link

Fix race condition between script load and rerenders #10

Closed timruffles closed 4 years ago

timruffles commented 4 years ago

Fixes #9 by:

  1. Only re-creating script tags when src changes - see below for why attributes currently weren't skipping any effect calls anyhow
  2. Rely on standard effect cleanup to handle unmounting. This works because
    1. cleanup runs when component unmounts - this is how is-unmounted is implemented anyhow (so we don't need the package)
    2. cleanup will be called for any previous value of src

Attributes currently weren't equal for any call to this hook. Objects are never strict equal unless they're the same object, never true for two reasons:

  1. the library uses ...attributes, which creates a new object (via __rest in the compiled output)
  2. users will pass in a new object each time if they're using React hooks correctly:
function MyComponent(props) {
  useScript({ src: props.src })
}

I've aded some tests to cover unmounting and onload.

How attributes could be supported

The above demonstrates that attributes are currently not equal for any two calls. I'm unsure how often that's a use-case people want though - I'd assume most would be loading a static src, and a small % a dynamic src which is handled correctly.

However, if it was a desired feature it would be necessary to:

  1. filter all non-serializable attributes out, and ensure a stable comparison.
    1. could be achieved with a stable JSON stringify - e.g https://www.npmjs.com/package/json-stable-stringify - but that's pretty slow.
  2. or pass an explicit script-change-key which the caller could define.

Finally - the same goal could be achieved with just src by adding superfluous query strings to the src to force a change, e.g const src = "https://cdn.com/some-script?k=" + someDynamicValue.

CarsonF commented 4 years ago

This adds a script tag to the DOM every time the hook is mounted. Previously this hook could be used in and out of the app without the fear of the spamming the dom with script tags.