fix: resolve Preboot race conditions

mgol commented 6 years ago

Please check if the PR fulfills these requirements
[x] The commit message follows our guidelines: https://github.com/angular/preboot/blob/master/CONTRIBUTING.md#commit-message-format
[ ] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been added / updated (for bug fixes / features)
What modules are related to this pull-request
[x] server side
[x] client side
[x] inline
[ ] build process
[ ] docs
[ ] tests
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) remove unused proxy imports from some test files

A bug fix via a refactor.

What is the current behavior? (You can also link to an open issue here)

See #82.

What is the new behavior (if this is a feature change)?

There should be no race conditions.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Yes, at least in the inline code handling as it no longer waits for the app root to be available so it cannot be fully put in <head>.

Other information:

Currently Preboot suffers from various race conditions:

It waits for the document.body to be available and then applies its logic. However, the application root may not be available at this point - see the issue

72.
Even if the application root exists when Preboot starts listening for events the server node may not be available in full yet. Since Preboot searches for nodes matching desired selectors inside of the existing serverNode, it may skip some of them if the server node hasn't loaded fully. This is especially common if the internet connection is slow.
Preboot waits for body in a tight loop, checking every 10 milliseconds. This starves the browser but may also be clamped by it to a larger delay (around 1 second); that's what happens in modern browsers if the tab where the page loads is inactive which is what happens if you open a link in a new tab in Chrome or Firefox. This means Preboot may start listening for events after Angular reports the app is stable and the PrebootComplete event fires. This then leaves the server node active, never transferring to the client one which makes for a broken site.

To solve it, we're doing the following:

Since we want to attach event listeners as soon as possible when the server node starts being available (so that it can be shallow-cloned) we cannot wait for it to be available in full. Therefore, we switched to delegated event handlers on each of the app roots instead of directly on specific nodes.
As we support multiple app roots, to not duplicate large inline preboot code, we've split getInlinePrebootCode into two parts: function definitions to be put in <head> and the invocation separate for each app root put just after the app root opening tag.
To maintain children offset numbers (otherwise selectors won't match between the client & the server) we've removed the in-app-root script immediately when it starts executing; this won't stop the execution. document.currentScript is a good way to find the currently running script but it doesn't work in IE. A fallback behavior is applied to IE that leverages the fact that start scripts are invoked synchronously so the current script is the last one currently in the DOM.
We've removed waitUntilReady as there's no reliable way to wait; we'll invoke the init code immediately instead.
We've switched the overlay used for freeing the UI to attach to document.documentElement instead of document.body so that we don't have to wait for body.
The mentioned overlay is now created for each app root separately to avoid race conditions.

Fixes #82 Fixes #72

BREAKING CHANGES:

When used in a non-Angular app, the code needs to be updated to use getInlineDefinition and getInlineInvocation; the previously defined getInlinePrebootCode has been removed. The getInlineDefinition output needs to be placed before any content user might interactive with, preferably in . The getInlineInvocation output needs to be put just after the opening tag of each app root. Only getInlineDefinition needs to have options passed. An example expected (simplified) layout in EJS format:

<html>
  <head>
    <script><%= getInlineDefinition(options) %></script>
  </head>
  <body>
    <app1-root>
      <script><%= getInlineInvocation() %></script>
      <h2>App1 header</h2>
      <div>content</div>
    </app1-root>
    <app2-root>
      <script><%= getInlineInvocation() %></script>
      <h2>App2 header</h2>
      <span>content</span>
    </app2-root>
  </body>
</html>

The refactor breaks working with frameworks that replace elements like AngularJS with UI Router as it no longer uses serverSelector and clientSelector but expects its clientNode to remain in the DOM.

TODO:

[x] Make it work in IE: add a fallback for missing document.currentScript.
[x] Make sure it works correctly for multiple app roots. ~~Maybe switch the selectors to be scoped to the concrete app root so that they don't interfere with each other?~~ (we're now attaching delegated handlers to the server node, not to document). This may be an issue as we now have separate code for each app root. We need to switch the logic to not use the appRoots array as each of the app roots is now handled separately.
[ ] Write some tests? (although the current code is not well tested either so that may require significant time investment)
[x] Decide whether to remove getInlinePrebootCode.

mgol commented 6 years ago

Another note to self: we still have the logic to handle multiple app roots in the recorder code; it should be changed to only work on one app root. Otherwise we have an O(n^2) complexity where n is the number of app roots, not to mention duplicate event registration.

mgol commented 6 years ago

PR updated; I still need to fix IE & refactor Preboot further a little so that it still works fine for multiple app roots but I addressed most comments otherwise.

mgol commented 6 years ago

PR rebased & updated; the logic has been rewritten, all it needs is tests & bug reports!

mgol commented 6 years ago

@CaerusKaru Feedback addressed. I also changed the commit message to mention the changed function names (without the PrebootCode infixes).

angular / preboot

fix: resolve Preboot race conditions #83

72.