SalesforceCommerceCloud / pwa-kit

React-based JavaScript frontend framework to create a progressive web app (PWA) storefront for Salesforce B2C Commerce.
https://developer.salesforce.com/docs/commerce/pwa-kit-managed-runtime/guide/pwa-kit-overview.html
BSD 3-Clause "New" or "Revised" License
283 stars 130 forks source link

[BUG] Duplicated request for async/defer scripts using react-helmet #1813

Closed sorioinc closed 4 months ago

sorioinc commented 4 months ago

Is there a way, other than react-helmet, of adding a third-party script tag in the head? I don't think it's a PWA problem, but a react-helmet one, however - since the package seems to be stale, maybe you can provide a solution?

I'm trying to improve the lighthouse score and adding async/defer to third party scripts result in an extra network request in client side.

CleanShot 2024-06-03 at 22 36 57@2x

Given the following jsx:

<Helmet>
  <script
    type="text/javascript"
    src="https://cdnjs.cloudflare.com/polyfill/v3/polyfill.min.js?features=Intl.DisplayNames,Intl.DisplayNames.~locale.en"
    defer
  />
</Helmet>

In the server, when running Helmet.renderStatic(): https://github.com/SalesforceCommerceCloud/pwa-kit/blob/0f5bbe375c9c54650e5463594ed327cfa437cf68/packages/pwa-kit-react-sdk/src/ssr/server/react-rendering.js#L301

It generates this output:

<script data-react-helmet="true" src="https://sdk.helloextend.com/extend-sdk-client/v1/extend-sdk-client.min.js" defer="true"></script>

So far, the difference between jsx and helmet is that the defer attribute is just a prop on the former, but defer="true" on the latter

Then, the extracted helmet scripts, are converted back to jsx in this line: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/0f5bbe375c9c54650e5463594ed327cfa437cf68/packages/pwa-kit-react-sdk/src/ssr/server/react-rendering.js#L343

And finally back to html with renderToString by react in here: https://github.com/SalesforceCommerceCloud/pwa-kit/blob/0f5bbe375c9c54650e5463594ed327cfa437cf68/packages/pwa-kit-react-sdk/src/ssr/server/react-rendering.js#L346

It generates the following html:

<script data-react-helmet="true" src="https://sdk.helloextend.com/extend-sdk-client/v1/extend-sdk-client.min.js" defer=""></script>

React renderToString sanitizes boolean properties to value ""

Then, when react-helmet picks up client-side, it tries to identify which scripts need to be replaced and remounted, in this line: https://github.com/nfl/react-helmet/blob/1b57ddb1524bab195df3eeabafef67768a6d2a15/src/HelmetUtils.js#L466

And basically asserts that defer="" !== defer="true", and so it decides to remove and re-add the script, thus having two downloads of the same script: One while document parses and then another when react-helmet is loaded

kevinxh commented 4 months ago

Have you tried to modify the original JSX like following? (you probably have tried this but i just want to confirm)

<Helmet>
  <script
    type="text/javascript"
    src="https://cdnjs.cloudflare.com/polyfill/v3/polyfill.min.js?features=Intl.DisplayNames,Intl.DisplayNames.~locale.en"
-    defer
+   defer="true"  
  />
</Helmet>

Another option

The pwa-kit-react-sdk does provide a mechanism to modify the entire HTML document shell during server side rendering.

There is a special SDK component called _document, see the default implementation here.

You could override this default by adding a file in your project /components/_document/index.jsx, and you can customize it like

const Document = (props) => {
    const {head, html, afterBodyStart, beforeBodyEnd, htmlAttributes, bodyAttributes} = props
    return (
        <html lang="en-US" {...htmlAttributes}>
            <head>
                <meta name="charset" content="utf-8" />
                <meta
                    name="viewport"
                    content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=5.0"
                />
                <meta name="format-detection" content="telephone=no" />
                {head.map((child) => child)}
+              <script
+                type="text/javascript"
+                 src="https://cdnjs.cloudflare.com/polyfill/v3/polyfill.min.js? 
+                  features=Intl.DisplayNames,Intl.DisplayNames.~locale.en"
+                 defer
+             />
            </head>
            <body {...bodyAttributes}>
                {afterBodyStart.map((child, i) => (
                    <React.Fragment key={i}>{child}</React.Fragment>
                ))}
                <div className="react-target" dangerouslySetInnerHTML={{__html: html}} />
                {beforeBodyEnd.map((child, i) => (
                    <React.Fragment key={i}>{child}</React.Fragment>
                ))}
            </body>
        </html>
    )
}

You should know that this puts the script tag out of React's reach and you should only use this method when you don't need to manipulate this script element on the client side.

Also, keep in mind adding script tags in the head may have performance implications and you should always make sure the addition of this script does not hurt your site performance.

kevinxh commented 4 months ago

I noticed your script seems to be adding a polyfill script to the site. The PWA does come with core-js polyfills that should be sufficient for most use cases, if you have special requirements, consider customizing the webpack configuration to include your polyfill, that will build the polyfills in the javascript bundle, so the browser doesn't have to make a separate network request for the polyfill js file.

sorioinc commented 4 months ago

Hi @kevinxh, thanks for the response.

Yes, I tried changing to defer="true", but renderToString will eventually sanitize.

Ah! That is the solution! I will make use of the _document component. Thank you!

Yes, for performance we are adding the async/defer tags, but looks like we have more options with the _document component.

The Intl.DisplayNames api is one of those core-js won't polyfill, so was the easiest way to go.

Thank you for your response, you saved me a lot of digging!