Skip to content

Conversation

timruffles
Copy link
Contributor

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.

expect(document.querySelectorAll('script').length).toBe(1);

renderHook(() => useScript({ src: 'http://scriptsrc/' }));
handle.rerender();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original test was equivalent to testing two separate components. This test is more akin to how the library is used.

If it was desired to avoid duplicate tags across components, I believe external state would be the correct solution in any case.

Copy link
Owner

@hupe1980 hupe1980 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thx

@hupe1980 hupe1980 merged commit 365056c into hupe1980:master Apr 2, 2020
@CarsonF
Copy link

CarsonF commented Apr 2, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testing for script element presence in DOM results in false positives/races
3 participants