Skip to content
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

Scripts with async in Head are executed twice #9070

Closed
jonkwheeler opened this issue Oct 14, 2019 · 20 comments · Fixed by codyogden/killedbygoogle#830 or #20748
Closed

Scripts with async in Head are executed twice #9070

jonkwheeler opened this issue Oct 14, 2019 · 20 comments · Fixed by codyogden/killedbygoogle#830 or #20748
Assignees
Milestone

Comments

@jonkwheeler
Copy link

jonkwheeler commented Oct 14, 2019

Bug report

Describe the bug

Scripts in the Head are run twice if marked async.

To Reproduce

Steps to reproduce the behavior:

  1. Create next app
  2. Create file at /static/log.js with console.log('hello'); inside
  3. Attach to a page (make sure to add the async attribute)
<NextHead>
  <script src="/static/log.js" async></script>
</NextHead>
  1. Run site
  2. See duplicate console.log in console.

Expected behavior

Scripts should only run once.

Screenshots

Screen Shot 2019-10-14 at 3 34 44 PM

System information

  • OS: macOS
  • Version of Next.js: latest 9.1.1
@mattjburrows
Copy link

I'm very new to nextjs, but this issue peaked my interest.

I've managed to take a quick look at the issue and found that it also happens for script tags with the defer attribute as well.

Testing it out locally, it looks like it happens when the client mounts and the script tag goes from <script src="/logs.js" async=""></script> to <script src="/logs.js" async="true"></script>. Not sure whether React is treating the async attribute as a boolean prop, but not sure how it ends up resolving it to true either.

I tracked it down to this line in the next-server/lib/head.tsx file. Removing the updateHead function from the handleStateChange prop, stopped the double call. I appreciate this wouldn't be a fix, as the updateHead is there for a reason.

Hopefully this little investigation can help track down the true source of the issue.

@Timer Timer modified the milestones: 9.1.x, 9.1.2 Oct 17, 2019
@developit
Copy link
Contributor

On the client, a NextHead update following initial hydration will change the value of the async prop from "" to true, which will cause the script to be re-executed.

A brute-force fix for this would be to read in the props when invoking cloneElement() on the ReactElements passed as children to NextHead, and coerce async / defer to "" for true, undefined for false:

    .map((c: React.ReactElement<any>, i: number) => {
      const key = c.key || i
      const props = { key }
      if ('async' in c.props) props.async = c.props.async ? '' : undefined
      if ('defer' in c.props) props.defer = c.props.defer ? '' : undefined
      return React.cloneElement(c, props)
    })

This is not a real solution either, but it might help narrow down the issue.

@Timer
Copy link
Member

Timer commented Oct 17, 2019

A temporary work around is to probably render this instead:

<script src="/static/log.js" async="async"></script>

@mattjburrows
Copy link

mattjburrows commented Oct 18, 2019

A temporary work around is to probably render this instead: <script src="/static/log.js" async="async"></script>

I gave that a try when looking into the issue and that still results in the script been re-executed (I should have mentioned that before).

Again, the script element goes from <script src="/logs.js" async=""></script> to <script src="/logs.js" async="async">.

If you end up going with @developit suggestion, it might be worth taking a look at other boolean attributes for script tags.

@dsmikeyorke
Copy link

I'm experiencing the same issue on a client's website. They are using Google Hire for job openings so we are loading the following script in

<script id="hire-embed-loader" defer async src="https://hire.withgoogle.com/s/embed/hire-jobs.js" />

On a client-side render, it loads as expected. On a server-side render, it runs twice resulting in two identical iframes. I tried adjusting async to async="async" which did not do the trick.

Has anyone found a solution that does not involve the temporary workarounds?

@jonkwheeler
Copy link
Author

jonkwheeler commented Nov 6, 2019

@dsmikeyorke a temporary workaround is to have a global var init at the top of the file...

/* see if this file already ran, if so, do nothing */
if (window.hasOwnProperty('myScriptNameInit') && window.myScriptNameInit){
  return;
}

/* else if it hasn't run, make that it has. */
window.myScriptNameInit = true;

While not ideal, this is a workaround to stop the file from running twice.

@fabb
Copy link
Contributor

fabb commented Nov 6, 2019

Maybe putting the scripts into _document.js resolves the issue temporarily since its contents should only be rendered during SSR and not being hydrated? I might be wrong, just guessing.

@Timer Timer modified the milestones: 9.1.3, 9.1.4 Nov 8, 2019
@dsmikeyorke
Copy link

Thanks for the feedback! I ended up wrapping the script in the following conditional so it only renders client-side:

{process.browser && <script id="hire-embed-loader" defer async src="https://hire.withgoogle.com/s/embed/hire-jobs.js" /> }

@jonkwheeler
Copy link
Author

@dsmikeyorke oh that's a nifty way to handle that. Any side effects to doing this on a ton of script inclusions?

@dsmikeyorke
Copy link

@jonkwheeler I only have this conditional on one script. I have not encountered any side effects and it cleared up the double load issue!

@jonkwheeler
Copy link
Author

jonkwheeler commented Nov 13, 2019

@dsmikeyorke I think I'm gonna go with @timneutkens suggestion here - #2177 (comment)

Very similar to what you said, and to what he said. He says not to assign it to a variable so it tree shakes away, so I'd love confirmation if this is bad or not. I can't imagine one var declaration harming anything, but wanted to double check.

const isBrowser = typeof window !== 'undefined'

.......

{isBrowser && <script src="1" />}

{isBrowser && <script src="2" />}

{isBrowser && <script src="3" />}

Edit:

Only thing that has worked for me is the following, and creating a var like const isBrowser = typeof window !== 'undefined'

{typeof window !== 'undefined' && <script src="1" />}

or

const isBrowser = typeof window !== 'undefined'

return (
  <NextHead>
    {isBrowser && <script src="2" />}
  </NextHead>
)

@Timer Timer modified the milestones: 9.1.4, 9.1.5 Nov 19, 2019
@fabyeah
Copy link

fabyeah commented Aug 12, 2020

Loading the script in the following Head component works for me so far. Fingers crossed.

const NonSSRHead = dynamic(
    () => import('next/head'),
    { ssr: false }
);

I'm using this workaround now. Though I still get a console warning Warning: Expected server HTML to contain a matching <header> in <div>. from time to time locally after hot reload. It then seems to break some styles. A page reload fixes it.

I changed some code and this stopped working. I am now using react-script-loader-hoc (https://github.com/sesilio/react-script-loader-hoc), which is based on this: https://usehooks.com/useScript/
Works so far, no more console errors about

You have included the Google Maps JavaScript API multiple times on this page. This may cause unexpected errors.

@alexandermendes
Copy link

Just came across this too, PR to hopefully fix ☝️

@Timer Timer modified the milestones: 10.x.x, iteration 15, iteration 14 Dec 8, 2020
@Timer Timer self-assigned this Dec 31, 2020
@Timer Timer added Upstream Related to using Next.js with a third-party dependency. (e.g., React, UI/icon libraries, etc.). and removed good first issue Easy to fix issues, good for newcomers type: needs investigation labels Jan 4, 2021
@Timer Timer modified the milestones: iteration 15, 10.x.x Jan 4, 2021
@Timer Timer added point: 3 and removed Upstream Related to using Next.js with a third-party dependency. (e.g., React, UI/icon libraries, etc.). labels Jan 4, 2021
Timer added a commit to Timer/next.js that referenced this issue Jan 4, 2021
This pull request correctly assigns boolean attributes for `<script />` to match the element as it is created by a server-side render.

Prior to this pull request, we'd double-execute `<script>` tags with the `async`, `defer`, or `nomodule` property.

---

Fixes vercel#9070
@kodiakhq kodiakhq bot closed this as completed in #20748 Jan 4, 2021
kodiakhq bot pushed a commit that referenced this issue Jan 4, 2021
This pull request correctly assigns boolean attributes for `<script />` to match the element as it is created by a server-side render.

Prior to this pull request, we'd double-execute `<script>` tags with the `async`, `defer`, or `nomodule` property.

---

Fixes #9070
@msafi
Copy link
Contributor

msafi commented Dec 22, 2021

In my case, I needed to specify id attribute on the tag to prevent Next from re-appending it multiple times.

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet