fix(helmet): fixes race condition with react-helmet

[e-l-i-s-e]

I'm proposing that when One App runs initClient that it only remove helmet scripts from the body and will no longer append those scripts to the head of the html doc. Appending the helmet scripts to the head will be done by Helmet, because Helmet will add the scripts to the head whether it runs before or after moveHelmetScripts.

Description

This PR changes moveHelmetScripts so that 1) it only queries the DOM for helmet scripts in the body rather than in the entire document, 2) no longer appends scripts to the head.

Motivation and Context

When One App middleware runs sendHtml on the server and creates an html doc it appends helmet scripts from the request to the html body. When the app is initialized on the client, the moveHelmetScripts function is invoked and it retrieves all scripts in the html doc, removes them from the body and adds them to the head.

The problem is that by the time this runs, it is possible for the Helmet component from react-helmet to have appended scripts to the head of the html doc. That duplicates the helmet scripts and will cause moveHelmetScripts to throw an error because it will try to remove all scripts it finds from the body without checking if they were found in the body.

There are 2 scenarios I've found where Helmet appends scripts before moveHelmetScripts runs:

1. When many modules are loaded on the page (race condition), moveHelmetScripts is firing after Helmet appends the scripts to the head. This seems to be due to the fact that it's invoked in an event listener waiting for DOMContentLoaded that can sometimes run afterreact-helmet updates the DOM (which is invoked in a componentWillMount lifecycle function). After Helmet appends scripts to the head, then moveHelmetScripts runs and grabs scripts from the body and the head. It will attempt to remove the head scripts from the body and then throws the error: Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

2. The second scenario when moveHelmetScripts throws an error is when a dev wants react-helmet to update the DOM ASAP and passes defer={false} to Helmet. https://github.com/nfl/react-helmet#readme
   If defer is false, then Helmet appends scripts to the head much earlier and will always cause moveHelmetScript to error if scripts have been added to the body on the server. Here is an example with the sample module, ssr-frank adding one helmet script with react-helmet:

<Helmet
  defer={false}
  onChangeClientState={(newState, addedTags, removedTags) => console.log('react-helmet - onChangeClientState', newState, 'added', addedTags, 'removed', removedTags)}
  script={[
      {
        type: 'application/ld+json',
        innerHTML: JSON.stringify({ '@context': 'https://schema.org' }),
       }
  ]}
/>

You can see that the script is duplicated and the function throws an error:

How Has This Been Tested?

I ran this using a demo route with one module and on a working route with multiple modules without passing the defer prop (so it's true) and passed it with the value false.

If we run ssr-frank with the following Helmet code added and the append command removed, you see that everything looks fine. moveHelmetScripts will remove the one script off the html body and then react-helmet will add the one script to the head:

If I pass defer={false} to Helmet, then the script will be added to the head before moveHelmetScripts runs. Everything works:

This shouldn't affect any other areas of the code.

Types of Changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (adding or updating documentation)
• Dependency update
• Security update

Checklist:

• My change requires a change to the documentation and I have updated the documentation accordingly.
• These changes should be applied to a maintenance branch.
• This change requires cross browser checks.
• Performance tests should be ran against the server prior to merging.
• This change impacts caching for client browsers.
• This change impacts HTTP headers.
• This change adds additional environment variable requirements for One App users.
• I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

This should have not noticeable impact on current users who do not see the DOM exception error. It will resolve that error for users who are currently seeing it and it will allow developers using One App to instruct Helmet to update the DOM earlier.

[CLAassistant]

All committers have signed the CLA.

[JAdshead]

PR Labeler currently has an issue with forks, I will add manually - TimonVS/pr-labeler-action#25

[10xLaCroixDrinker]

@e-l-i-s-e thank you for your contribution! after looking at the issue, we were thinking something more like this: c0e17b1

This way we can make sure that the scripts are moved immediately instead of waiting for react-helmet to do it

Is that a change you'd be amenable to make?

[e-l-i-s-e]

@10xLaCroixDrinker I think that you both raise a very important point. I want to think for a bit more about the solution you have proposed and test it out on a few things if you don't mind. I'll get back to you shortly. Thanks again for providing a possible solution!

[e-l-i-s-e]

Hello, @10xLaCroixDrinker and @JAdshead -- I have a few questions that I haven't yet been able to answer yet:

1. The Holocron module in our app that adds Helmet scripts was rendering 4 or 5 times before the domContentLoaded event was firing (and invoking moveHelmetScripts) with the initial solution I proposed. So react-dom must be using a different strategy to determine when it can hydrate that allows it to run slightly sooner? Would we want to imitate whatreact-dom is doing? Or do you think that the difference is insignificant/not worth the effort required?
2. Shouldn't delaying reactDOM.render() or reactDOM.hydrate() increase TTI by some small amount? I assumed that it would, but I wasn't able to see a noticeable difference when I measured the speed of the pages I was testing. Maybe the difference really is just a matter of just a few milliseconds?

If you aren't concerned with that possible impact, then I think the proposed solution is great and I'll push up changes.

[e-l-i-s-e]

I'm thinking that it might be slightly more optimal to compare scripts queried from the body and head and then append them if needed rather than delay hydrating the app. I'll push up that change and see what you both think.

[JAdshead]

Hello, @10xLaCroixDrinker and @JAdshead -- I have a few questions that I haven't yet been able to answer yet:

1. The Holocron module in our app that adds Helmet scripts was rendering 4 or 5 times before the domContentLoaded event was firing (and invoking moveHelmetScripts) with the initial solution I proposed. So react-dom must be using a different strategy to determine when it can hydrate that allows it to run slightly sooner? Would we want to imitate whatreact-dom is doing? Or do you think that the difference is insignificant/not worth the effort required?
2. Shouldn't delaying reactDOM.render() or reactDOM.hydrate() increase TTI by some small amount? I assumed that it would, but I wasn't able to see a noticeable difference when I measured the speed of the pages I was testing. Maybe the difference really is just a matter of just a few milliseconds?

If you aren't concerned with that possible impact, then I think the proposed solution is great and I'll push up changes.

I suspect that this is because react render is preventing the domContentLoaded from firing as it updates the dom. if we wait for domContentLoaded to fire before calling render then this should not be an issue. I don't foresee any measurable impact to users for TTI.

[e-l-i-s-e]

Hi @JAdshead and @10xLaCroixDrinker, have you had a chance to look at the new code change afefbc7?

[e-l-i-s-e]

Hi @JAdshead, It makes sense that more helmet scripts could be added from client bundles. So would it be sufficient to check if any helmet scripts are present in the head in that case? That would mean that another library added them, correct? Since one-app puts the helmet scripts in the body and wouldn't have appended any to the head until this function is evoked.
Here's the new option: 48b7c42

[e-l-i-s-e]

Hi @JAdshead and @10xLaCroixDrinker, I thought I'd circle back and see if you had a chance to look my recent comment and commit -- #534 (comment).

Do either or you foresee a problem with that simpler approach? Then we won't need to worry about delaying the rendering/hydrating of the app. If you would prefer to go with your initial proposal, I'm happy to use that instead.

[JAdshead]

I am happy to accept this. Just update the comment to match the flow.

[e-l-i-s-e]

Thanks, @JAdshead. I updated the comments and it's ready for re-review.