Handle <script async> transparently

[squadette]

I believe that this commit fixes #290, transparently handling <script async> tag. If the document was already loaded, the DOMContentLoaded event handler no longer fires, so we just execute it directly, the same way as jQuery does in $.ready().

---

This change is 

[coveralls]

Coverage remained the same at ?% when pulling d164e75 on squadette:script-async into e86090f on shakacode:master.

[justin808]

Possibly related links:

• https://github.com/turbolinks/turbolinks/pull/274/files
• turbolinks:load event in deferred scripts no longer always trigger on page load in 5.0.1 turbolinks/turbolinks#278
• async/defer turbolinks/turbolinks#281

@nateberkopec, any chance that you can help us review this one?

@alexeyMohnatkin can you provide more examples of this and documentation?

Can you demonstrate the fix working and not working? Maybe modify the spec/dummy app and add the async tags?

How about https://github.com/shakacode/react-webpack-rails-tutorial? Can you demonstrate what would be the effect with and without this little change?

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed, 1 unresolved discussion (waiting on @squadette)

---

node_package/src/clientStartup.js, line 214 at r1 (raw file):

            'NOT USING TURBOLINKS: DOMContentLoaded event, calling reactOnRailsPageLoaded',
          );
          reactOnRailsPageLoaded();

@alexeyMohnatkin is this related? an example? https://github.com/turbolinks/turbolinks/pull/274/files

Previous code always waited for the event.

Any more comments on why this fix works?

---

Comments from Reviewable

[squadette]

hi,

first, you're summoning a random guy who has similar last name to mine. :)

second, yes, all three turbolinks links are trying to solve the same issue. (And that issue is also solved by jQuery's ready() function: https://github.com/jquery/jquery/blob/bd984f0ee2cf40107a669d80d92566b8625b1e6b/src/core/ready.js#L67).

third, I believe that this issue needs to be fixed separately for turbolinks and non-turbolinks cases. I don't use turbolinks so I'm not trying to even try handling it: looks like they should solve it on their side (if needed).

fourth, it's quite hard to demonstrate that it's working, because you cannot make sure that async script tag finishes execution reliably before or reliable after DOMContentLoaded event fires.

We know that if you add async tag to current react_on_rails-based project the page does not work (so, nobody probably does that). We know that this change does not break ordinary script tag. Seems that the only way to check if async tag works is to test it in real world.

...to be continued.

[justin808]

@squadette Looking forward to a heads up on this one. I just want to be sure we don't break any existing code.

[nateberkopec]

Will this break if the readyState is interactive?

[squadette]

I don't think so. This code was lifted straight from jQuery 3, I assume they're doing the right thing. The rest of the condition seems to handle "not loading", that is "interactive".

…

On Thu, Jun 21, 2018 at 3:14 PM Nate Berkopec ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In node_package/src/clientStartup.js <#1099 (comment)> : > @@ -196,6 +196,27 @@ export function clientStartup(context) { debugTurbolinks('Adding DOMContentLoaded event to install event listeners.'); + window.setTimeout(() => { + if (!turbolinksInstalled() || !turbolinksSupported()) { + if (document.readyState === 'complete' || Will this break if the readyState is interactive? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1099 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAL7CeUhUz2MaEGbTtCHsXjwYG5B3dfks5t-5xFgaJpZM4Ug7gK> .

-- Alexey Makhotkin

[justin808]

@nateberkopec Do you feel OK with @squadette's suggestion?

@squadette, is there any chance that we could make an adjustment to the /spec/dummy app to verify this? If you tell me manually, I'll try it out. Like set some script to async?

[justin808]

Maybe changing these 2 lines

  <%= javascript_pack_tag('vendor-bundle', 'data-turbolinks-track': true) %>
  <%= javascript_pack_tag('app-bundle', 'data-turbolinks-track': true) %>

Also, how does the browser and webpack know that the second file app-bundle has to load after vendor-bundle.

[justin808]

Closed in #1107.

[squadette]

@justin808, I've missed your question about splitted bundles. I don't know exactly how Webpack splitted bundles work, but if they depend on global names then splitted bundles will not work with async (but will work with defer).