Asynchronously load note embeds #11

reefdog · 2016-10-11T03:55:58Z

The note embed code looks like this:

<div id="DC-note-1234" class="DC-note-container"></div>
<script src="//assets.documentcloud.org/notes/loader.js"></script>
<script>
  dc.embed.loadNote('//www.documentcloud.org/documents/4567-doc-title/annotations/1234.json');
</script>

It depends on render-blocking syncronous script loading:

loader.js loads remotely (blocking)
loader.js uses document.write to add note_embed.js to the DOM (bad practice and being deprecated)
note_embed.js loads remotely (blocking)
dc.embed.loadNote called via inline script

For backwards compatibility with existing embed codes, we're stuck with 1 and 4, but we can fix 2 and 3.

After this PR is merged:

loader.js redefines dc.embed.loadNote as a queuing function
loader.js uses document.createElement (instead of document.write) to add note_embed.js to the DOM
- and is smart enough to only add it if necessary
- and adds it with the async attribute so page rendering can continue
When note_embed.js finishes loading, queued notes are loaded
If note_embed.js has already run when loader.js does (like in pjax page changes where both from/to pages contain note embeds) the note loads immediately instead of being queued

There's also the addition of container-less note embeds at invocation site, because (1) I thought it was existing functionality that I'd broken and (2) @knowtheory accidentally challenged me to do it. So this is now a supported – if slow and discouraged – embed code:

<script src="//assets.documentcloud.org/notes/loader.js"></script>
<script>
  dc.embed.loadNote('//www.documentcloud.org/documents/4567-doc-title/annotations/1234.json');
</script>

One downside: we now require IE8+. loader.js (which doesn't include jQuery) uses document.querySelector to avoid redundant DOM insertions of note_embed.css and note_embed.js. If that's a problem, I can tweak it so that only supported browsers avoid redundant insertions, and older browsers get the existing behavior.

* Insert `note_embed.js` using `document.createElement` instead of `document.write`, and flag it `async` * Only write one copy of `note_embed.(js|css)` to page * Queue up notes to execute after `note_embed.js` is availble * Add nuclear option for creating the target DIV for a note that has an inline script but no container # Conflicts: # dist/note_embed.js

* Check for presence of `actuallyLoadNote` function instead of a redundant boolean * Don’t nest the note-loading utilities in an object # Conflicts: # dist/note_embed.js

We don’t use any dataURIs anymore

knowtheory · 2016-10-11T03:58:50Z

This is awesommmmme.

Nah, screw IE8. We can pull numbers if we need to but looking at Google analytics, IEs <10 have dwindled sufficiently.

knowtheory · 2016-10-11T04:01:54Z

Now, loader.js w/ document.write causes problems when dynamically inserted (for example in the case of pjax pages). Does loader.js w/ createElement/appendChild suffer the same problem?

NB: Because of cross-origin restrictions, you can’t test this when loading the test page via `file:///`

* Assume we’re testing off `notes.dcloud.org` and `notes-cors.dcloud.org` (to be formalized with better test rig soon) * Have new and old versions of test page for regression testing (super super hacky rn) * Localize oEmbed endpoint Committing so it’s not sitting on my machine during vacation.

reefdog · 2016-10-18T21:09:43Z

Now, loader.js w/ document.write causes problems when dynamically inserted (for example in the case of pjax pages). Does loader.js w/ createElement/appendChild suffer the same problem?

Yes, but for a different reason.

Our current vended embed code is dependent on this synchronous execution:

<script src="loader.js"></script>
<script>dc.embed.loadNote()</script>

What we ✅ are able to do: change loader.js so that dc.embed.loadNote() becomes a queueing function; writes note_embed.js with createElement instead of document.write; and loads notes asynchronously.

What we ❌ aren't able to do: survive anything that causes the inline script block containing dc.embed.loadNote() to be executed before loader.js.

Unfortunately, what I've discovered:

Pjax removes <script src> tags from the content and inserts them into <head>, which breaks the guarantee that they'll load and execute synchronously before the inline script block. This is by design and unlikely to change.
jQuery seems to do something similar, but I haven't nailed down its behavior precisely. In any case, fetching a vanilla note embed code with $.get() and inserting it onto page with .html() causes the inline script block to be executed before loader.js has loaded/executed.

So effectively: people who use Pjax (or otherwise pull in content via Ajax) who really want to embed our notes will need to add our <script src="loader.js"></script> to their templates to make sure the loader is present on every page before the content is fetched/inserted. Gross but evidently true!

* Organize things into v1.0 and v1.1 directories for distinction * Add pjax/ajax/oembed tests

We’ve effectively deprecated IE8 support anyway now that we use `document.querySelector` in the loader. If we ever need to reenable IE8 support, we’ll have to downgrade to jQuery 1.x. NB: Requires us to point ot jQuery 2.2.4 on Pjax test pages since Pjax doesn’t support jQuery 3 yet.

reefdog · 2016-10-18T23:01:29Z

Happy enough with this to merge, make necessary platform changes (moving the loader over, essentially), and deploy. Passes browser testing and there're no regressions I can find. It doesn't magically make our existing embed codes work in pjax-y environments, but now we know why.

@knowtheory do you need to take a look at this, or can I merge (once I've made the necessary platform changes too)?

Old world: `dc.embed.loadNote` defined in note_embed.js and loads note. New world: `dc.embed.loadNote` defined in loader.js and queues note, firing `dc.embed.actuallyLoadNote` (defined in note_embed.js) when ready. Problem: new loader.js and new note_embed.js codependent. With this change, note_embed.js aliases `loadNote` to `actuallyLoadNote` if the former isn’t defined, so getting the old loader.js but the new note_embed.js still works.

These being top-level dependencies is inaccurate, because the platform doesn’t use the src files directly, it only uses the `dist` versions built by jammit.

This reverts commit 5a1a866.

Otherwise, if newlines are removed, the first `//` hides everything after it

Discussion and detail here: documentcloud/documentcloud-notes#11

commit b0cd03e Author: Justin Reese <justin@justinreese.com> Date: Tue Feb 28 11:38:39 2017 -0600 Replace Gotham with standard font stack commit d0578b2 Author: Justin Reese <justin@justinreese.com> Date: Wed Feb 22 13:40:53 2017 -0600 Update loader example commit 17e992f Author: Justin Reese <justin@justinreese.com> Date: Wed Feb 22 13:38:41 2017 -0600 Support passing a note object (JSON or native) Before: `dc.embed.loadNote` (and `dc.embed.immediatelyLoadNote`) take a string URL as the first argument. After: That still works, but you can also pass a valid native JS object or JSON string representation of the note. This is a more rigid embed, but bypasses our app servers and only pulls from the CDN. Good for high-performance embeds. commit ed60a48 Author: Justin Reese <justin@justinreese.com> Date: Wed Feb 22 13:32:22 2017 -0600 Indicate that oEmbed works with 1.1-async loader commit 2960a6e Author: Justin Reese <justin@justinreese.com> Date: Fri Nov 4 17:16:18 2016 -0500 Fix with old oEmbed loader, update test loader documentcloud/documentcloud#420 commit efe1da7 Author: Justin Reese <justin@justinreese.com> Date: Fri Nov 4 17:14:41 2016 -0500 Update 1.1 embed code samples commit 9143025 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 31 16:24:51 2016 -0500 Pull `addEventListener` polyfill from test loader commit 9dce485 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 31 15:17:01 2016 -0500 `actuallyLoadNote` → `immediatelyLoadNote` Better describes its behavior and intent. commit 179fe43 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 31 12:18:23 2016 -0500 Move `loadNote` aliasing closer to original fn commit c1c58df Merge: 9902948 1ffe1d6 Author: Justin Reese <justin@justinreese.com> Date: Fri Oct 28 11:52:07 2016 -0500 Merge pull request #11 from documentcloud/async-load Asynchronously load note embeds commit 1ffe1d6 Author: Justin Reese <justin@justinreese.com> Date: Fri Oct 28 11:47:03 2016 -0500 Use multiline comments in loader Otherwise, if newlines are removed, the first `//` hides everything after it commit 5dbb7e2 Author: Justin Reese <justin@justinreese.com> Date: Fri Oct 28 11:27:55 2016 -0500 Revert "Make all bower dependencies → devDependencies" This reverts commit 5a1a866. commit 5a1a866 Author: Justin Reese <justin@justinreese.com> Date: Fri Oct 28 11:24:51 2016 -0500 Make all bower dependencies → devDependencies These being top-level dependencies is inaccurate, because the platform doesn’t use the src files directly, it only uses the `dist` versions built by jammit. commit 7722657 Author: Justin Reese <justin@justinreese.com> Date: Fri Oct 28 11:09:17 2016 -0500 Alias old `loadNote` to new for back compatibility Old world: `dc.embed.loadNote` defined in note_embed.js and loads note. New world: `dc.embed.loadNote` defined in loader.js and queues note, firing `dc.embed.actuallyLoadNote` (defined in note_embed.js) when ready. Problem: new loader.js and new note_embed.js codependent. With this change, note_embed.js aliases `loadNote` to `actuallyLoadNote` if the former isn’t defined, so getting the old loader.js but the new note_embed.js still works. commit 832cf7b Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 25 17:30:33 2016 -0500 Protocol-agnostic CORS test URLs commit 49a6e3d Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 25 14:16:54 2016 -0500 Remove unnecessary port on test files commit c1b9701 Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 17:51:00 2016 -0500 Add status emoji to test links commit a5b3e63 Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 17:47:42 2016 -0500 Use shorthand document ready func on test pages commit d9afb93 Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 17:47:16 2016 -0500 Update jQuery to 3.x now that IE8 isn’t supported We’ve effectively deprecated IE8 support anyway now that we use `document.querySelector` in the loader. If we ever need to reenable IE8 support, we’ll have to downgrade to jQuery 1.x. NB: Requires us to point ot jQuery 2.2.4 on Pjax test pages since Pjax doesn’t support jQuery 3 yet. commit b4bb56a Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 17:43:05 2016 -0500 Remove unused old_loader jammit config commit babd90d Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 17:35:05 2016 -0500 Remove unused iframe embed content commit e155a73 Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 16:59:25 2016 -0500 Remove logging, clean up comments/formatting commit b1585be Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 16:51:04 2016 -0500 Maybe don’t declare local vars globally commit 9c805ea Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 18 16:43:02 2016 -0500 Update test rig * Organize things into v1.0 and v1.1 directories for distinction * Add pjax/ajax/oembed tests commit 298ea16 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 17 10:37:28 2016 -0500 Revert removal of datauri version of CSS commit a7ffd6f Author: Justin Reese <justin@justinreese.com> Date: Wed Oct 12 18:02:36 2016 -0500 Very WIP changes to test pages * Assume we’re testing off `notes.dcloud.org` and `notes-cors.dcloud.org` (to be formalized with better test rig soon) * Have new and old versions of test page for regression testing (super super hacky rn) * Localize oEmbed endpoint Committing so it’s not sitting on my machine during vacation. commit 99979fa Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 11 14:59:54 2016 -0500 Add test case for inserting embed code with Ajax NB: Because of cross-origin restrictions, you can’t test this when loading the test page via `file:///` commit 42dc2ff Author: Justin Reese <justin@justinreese.com> Date: Tue Oct 11 12:19:28 2016 -0500 Style/layout test page commit 883d6fb Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 10 23:57:35 2016 -0500 Align a few assignments commit eab03f6 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 10 23:57:29 2016 -0500 Localize an accidental global var commit d715732 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 10 22:23:28 2016 -0500 Don’t generate datauri version of CSS We don’t use any dataURIs anymore commit 81371d6 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 10 22:20:02 2016 -0500 Clean up loader changes a little * Check for presence of `actuallyLoadNote` function instead of a redundant boolean * Don’t nest the note-loading utilities in an object # Conflicts: # dist/note_embed.js commit e900507 Author: Justin Reese <justin@justinreese.com> Date: Mon Oct 10 17:00:25 2016 -0500 Rewrite loader to be asynchronous * Insert `note_embed.js` using `document.createElement` instead of `document.write`, and flag it `async` * Only write one copy of `note_embed.(js|css)` to page * Queue up notes to execute after `note_embed.js` is availble * Add nuclear option for creating the target DIV for a note that has an inline script but no container # Conflicts: # dist/note_embed.js commit 9902948 Author: Ted Han <ted@knowtheory.net> Date: Mon Sep 26 10:30:34 2016 -0700 add definitiveness commit 6152599 Author: Ted Han <ted@knowtheory.net> Date: Tue Jul 5 11:47:59 2016 -0700 Don't step on jQuery if other jQueries have been loaded and have already been noConflict'd commit 8eb6ddc Author: Justin Reese <justin@justinreese.com> Date: Sun Mar 13 10:29:10 2016 -0600 Whackamole: Don't use borders on links

reefdog added 3 commits October 10, 2016 22:39

Clean up loader changes a little

81371d6

* Check for presence of `actuallyLoadNote` function instead of a redundant boolean * Don’t nest the note-loading utilities in an object # Conflicts: # dist/note_embed.js

Don’t generate datauri version of CSS

d715732

We don’t use any dataURIs anymore

reefdog added 6 commits October 10, 2016 23:57

Localize an accidental global var

eab03f6

Align a few assignments

883d6fb

Style/layout test page

42dc2ff

Add test case for inserting embed code with Ajax

99979fa

NB: Because of cross-origin restrictions, you can’t test this when loading the test page via `file:///`

Revert removal of datauri version of CSS

298ea16

reefdog added 8 commits October 18, 2016 16:43

Update test rig

9c805ea

* Organize things into v1.0 and v1.1 directories for distinction * Add pjax/ajax/oembed tests

Maybe don’t declare local vars globally

b1585be

Remove logging, clean up comments/formatting

e155a73

Remove unused iframe embed content

babd90d

Remove unused old_loader jammit config

b4bb56a

Use shorthand document ready func on test pages

a5b3e63

Add status emoji to test links

c1b9701

reefdog added 6 commits October 25, 2016 14:16

Remove unnecessary port on test files

49a6e3d

Protocol-agnostic CORS test URLs

832cf7b

Make all bower dependencies → devDependencies

5a1a866

These being top-level dependencies is inaccurate, because the platform doesn’t use the src files directly, it only uses the `dist` versions built by jammit.

Revert "Make all bower dependencies → devDependencies"

5dbb7e2

This reverts commit 5a1a866.

Use multiline comments in loader

1ffe1d6

Otherwise, if newlines are removed, the first `//` hides everything after it

reefdog merged commit c1c58df into master Oct 28, 2016

reefdog deleted the async-load branch October 28, 2016 16:53

reefdog added a commit to documentcloud/documentcloud that referenced this pull request Oct 28, 2016

Update note loader to async queuing version

fc90593

Discussion and detail here: documentcloud/documentcloud-notes#11

reefdog mentioned this pull request Oct 31, 2016

Load embed JS async and without document.write documentcloud/documentcloud#418

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asynchronously load note embeds #11

Asynchronously load note embeds #11

reefdog commented Oct 11, 2016 •

edited

Loading

knowtheory commented Oct 11, 2016

knowtheory commented Oct 11, 2016

reefdog commented Oct 18, 2016 •

edited

Loading

reefdog commented Oct 18, 2016 •

edited

Loading

Asynchronously load note embeds #11

Asynchronously load note embeds #11

Conversation

reefdog commented Oct 11, 2016 • edited Loading

After this PR is merged:

knowtheory commented Oct 11, 2016

knowtheory commented Oct 11, 2016

reefdog commented Oct 18, 2016 • edited Loading

reefdog commented Oct 18, 2016 • edited Loading

reefdog commented Oct 11, 2016 •

edited

Loading

reefdog commented Oct 18, 2016 •

edited

Loading

reefdog commented Oct 18, 2016 •

edited

Loading