If page is interactive, fire pageLoaded() immediately

[nateberkopec]

When using Turbolinks with the async script attribute, Turbolinks may
execute after DOMContentLoaded. To fix this, we only attach a handler
to DOMContentLoaded if the readystate is not "loading" (that is to say,
it is "interactive" or "complete"). Otherwise, we fire the pageLoaded
event immediately without installing the handler.

This changes Turbolinks interaction with defer scripts. After this
change, an async Turbolinks script may execute before defered
scripts. This is because the browser executes all defer scripts before
the DOMContentLoaded event. Previously, Turbolinks would always execute
after these scripts, but may now execute before them in some cases
depending on when Turbolinks has finished downloading.

[nateberkopec]

Not sure the whole caveat about "now Turbolinks can execute before deferred scripts" even matters, but figured I'd mention it.

Closes #266

[javan]

I've been thinking more on this and now I'm not sure what the right thing to do is. turbolinks:load more or less maps to when DOMContentLoaded would have fired in a non-Turbolinks app. With this change, turbolinks:load could fire any time initially depending on how and when Turbolinks is loaded. It could also fire if Turbolinks is stopped and then started again (e.g. Turbolinks.controller.stop(); doSomething(); Turbolinks.controller.start())

[nateberkopec]

It could also fire if Turbolinks is stopped and then started again (e.g. Turbolinks.controller.stop(); doSomething(); Turbolinks.controller.start())

This could be easily fixed with just tracking the state somewhere to make sure it only goes off once, right?

turbolinks:load could fire any time initially depending on how and when Turbolinks is loaded

This is interesting. Is this really a bad thing, though? If someone's just using JQuery and $(document).ready hooks, adding async would do the same thing.

[javan]

If someone's just using JQuery and $(document).ready hooks, adding async would do the same thing.

The difference is $(document).ready is a function that takes a callback, not an event. Events should indicate that something happened, and firing turbolinks:load at an indeterminate time after DOMContentLoaded breaks that contract.

[nateberkopec]

It depends on what you think turbolinks:load means, I guess. If it means "the DOM is ready and Turbolinks has been evaluated" then nothing has changed. The only thing that's changed is "deferred scripts may not have been executed".

could fire any time initially

To be clear, it can fire any time after the document has finished parsing (readystate interactive). It can't fire before that (still). It can also now fire before other deferred scripts. If I marked Turbolinks.js as async, "this event may fire as soon as it can, which may be after page load" is exactly the behavior I would expect.

[sstephenson]

+1. Agree the initial turbolinks:load event should be dispatched immediately if the page is interactive.

I don’t think apps should use turbolinks:load as a general-purpose hook for installing JS behavior, but I’ll accept this concession to the way most apps are likely using the event.

[javan]

From the README:

turbolinks:load fires once after the initial page load, and again after every Turbolinks visit.

This change assumes that turbolinks will always be loaded in the initial page's js (synchronously, defer'd, or async), but it's possible that it could be loaded in other ways. Imagine this pathological scenario:

// <script src="main.js">
setTimeout(function() {
  lazyLoad("turbolinks.js")
}, 10000) 

Would you expect a turbolinks:load then?

Here's one way Turbolinks could handle it:

--- a/src/turbolinks/controller.coffee
+++ b/src/turbolinks/controller.coffee
@@ -17,9 +17,18 @@ class Turbolinks.Controller
   start: ->
     if Turbolinks.supported and not @started
       addEventListener("click", @clickCaptured, true)
-      addEventListener("DOMContentLoaded", @pageLoaded, false)
+      @startHistory() # Start history to set initial `@location`
+      switch document.readyState
+        when "loading"
+          # Page is still loading, fire `turbolinks:load` when it's interactive
+          addEventListener("DOMContentLoaded", @pageLoaded, false)
+        when "interactive"
+          # Page is interactive, fire `turbolinks:load` now
+          @pageLoaded()
+        else
+          # Page is loaded, don't fire `turbolinks:load`
+          @lastRenderedLocation = @location
       @scrollManager.start()
-      @startHistory()
       @started = true
       @enabled = true

[nateberkopec]

I guess you're counting on the fact that the turbolinks javascript tag itself will keep the document in interactive state until it's finished executing? Is that really the case? I don't think a Javascript subresource "stops delaying the load event" while it's executing.

Fetching an external script must delay the load event of the element's document until the task that is queued by the networking task source once the resource has been fetched (defined above) has been run.

So, a Turbolinks script with async and no other subresources on the page would probably hit the else condition here, which is probably not what you intended.

[nateberkopec]

Would you expect a turbolinks:load then?

My answer to that is "yeah, probably". I mean, what are the odds someone is inserting Turbolinks onto a page and counting on turbolinks:load not firing?

[javan]

Cool, yeah, I'm just being difficult. 😬

[javan]

I think @startHistory() needs to be called before this condition so @location is set for @pageLoaded() to use.

[nateberkopec]

👍 moved startHistory

[nateberkopec]

Proposing a README change as well to be more specific about when turbolinks:load fires, happy to workshop the phrasing.

[javan]

👍 to the README change, but it's conflicting with master somehow. Mind fixing it up?

[nateberkopec]

Removed the whitespace removal and it looks good now.

[javan]

❤️

[javan]

Turbolinks 5.0.1 is out!

[glebm]

@nateberkopec #274 (comment)
My answer to that is "yeah, probably". I mean, what are the odds someone is inserting Turbolinks onto a page and counting on turbolinks:load not firing?

The odds are "definitely" because turbolinks:load not firing had to be accounted for to make Turbolinks 5.0.0 work with async scripts.

[nateberkopec]

@glebm That comment was in regard to somebody injecting Turbolinks into a page long after initial load (i.e. not async or defer).

I still don't think we actually broke anything for you.

[glebm]

I still don't think we actually broke anything for you.

@nateberkopec What do you mean? It was working and now it's not (well I've patched it on master but then people will run bundle update and because only PATCH has been bumped, it'll break for them if they don't also update Thredded).

If you believe that it wasn't working before either you can check that very easily: https://github.com/thredded/thredded#development
Also there is at least 20 forums with over 400,000 total posts currently running Thredded, some using Turbolinks v5.0.0.

PS It's 3:30AM here so I'm logging off for a while

[maia]

@nateberkopec I'm also having issues with the new bahavior, and am unsure how to fix it. Until now I've loaded certain page elements that take longer to load due to db queries via ajax - I added link_towith remote: true and class: "something" to these elements, and on turbolinks:load I called $('.something').click().

Now as == javascript_include_tag 'application', 'data-turbolinks-track': 'reload', async: production?frequently will load jQuery, turbolinks etc. after DOMContentLoaded, turbolinks:load does not always fire on initial page load.

If anyone has a suggestion how to properly load page elements automatically via ujs once the page itself is loaded, I'd be thankful. Using DOMContentLoaded to load them did cause side effects, as I also rely on other javascripts being loaded first.

So the question is: which event can we rely on that will trigger after all javascript has been loaded async, even on the initial page load? Until turbolinks 5.0.0 it was turbolinks:load, what is it now?

[nateberkopec]

@maia Upgrade to >= 5.0.2

[mindreframer]

@nateberkopec latest published version as of today is 5.0.1
https://rubygems.org/gems/turbolinks

??? is this still a not fixed bug?

[nateberkopec]

@mindreframer https://rubygems.org/gems/turbolinks-source

[mindreframer]

@nateberkopec great, thx!