-
-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(useScript): variable scope bug that stops scripts being unmounted #103
Conversation
🦋 Changeset detectedLatest commit: b7052a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Needs a changeset |
I've never used changesets before and can't see any that exist in the repo but I clicked the link above and it seems to have created one |
@webdevian can you rebase this against main? |
@JoviDeCroock I think that is done now? |
@webdevian you have failing tests |
Ok, the 2 failing tests aren't working in main either. The existing tags aren't being reused, it is adding duplicate tags to the head
I will make some changes to useScript and to the tests |
@webdevian They do work on main https://github.com/0no-co/hoofd/actions/runs/12714681359/job/35445316145 |
…o check all scripts present in head
They pass, but they are not actually testing that existing script tag is re-used, just that a new tag is present in the head. See my updated tests for more explicit checks on number of script elements, and checking the attributes without a full string comparison. I've also had to change the hook to allow preExistingElements[0] to be modified |
I've also added a couple of tests for the unmount |
I ran into this bug when testing for scripts being removed from the DOM.
When the script element is first created (L32) it was not assigned to the
script
variable that the useEffect return function is using to remove the element (L57)