Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New async loader incompatible with multiple sites #420

Closed
reefdog opened this issue Nov 2, 2016 · 4 comments

Comments

Projects
None yet
1 participant
@reefdog
Copy link
Contributor

commented Nov 2, 2016

After merging and deploying #418, we immediately noticed the embeds breaking on some sites (but not others), and so rolled back to the old loader for further investigation.

Essentially, the new loader does this (embedding a document in this example):

  1. Is DV.immediatelyLoadDocument (defined in viewer.js) available? If so, fire it
  2. If not, queue the document, insertJavaScript(), and insertStylesheet()
  3. insertJavaScript() inserts viewer.js with an onload callback that loads all queued documents, unless it finds viewer.js already in the DOM

The model falls apart at the points when the embedding site has already stuck viewer.js in the DOM before loader.js even runs, because in that case:

  1. If viewer.js has finished loading (thus making DV.immediatelyLoadDocument available), then the document loads correctly, but we never run insertStylesheet(), so the CSS is missing
  2. If viewer.js hasn't finished loading, then we queue the document, but then insertJavaScript() notices that viewer.js is already in the DOM, skips adding it, and skips adding the onload callback, so the queued documents never load

The correct behavior:

  1. Insert the CSS regardless of whether we're queuing this document
  2. Attach the queue-loading onload callback to viewer.js regardless of whether we added it to the DOM or found it there already

@reefdog reefdog added the bug label Nov 2, 2016

@reefdog reefdog self-assigned this Nov 2, 2016

@reefdog reefdog closed this in a3efe49 Nov 2, 2016

@reefdog

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2016

Discovered that the changes to the viewer (1, 2) were causing problems on Forbes articles like this one. Reverted, again, to the old loader and old viewer for further research.

Updated: Forbes (and certainly others) cached the oEmbed response, which inlines the old oEmbed loader but points to the remote viewer.js. So need to ensure any changes to viewer.js are compatible with the old oEmbed loaders. (Already verified they're compatible with the old normal loaders.)

@reefdog reefdog reopened this Nov 2, 2016

@reefdog

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Found the incompatibility between old oEmbed loader and new viewer.

Old oEmbed loader defined DV.load as a queuing function (like the new loader does) and also fired off a queue-loading function that checked every 500ms to see if the viewer had loaded (by presence of DV.viewers) and, if so, fired DV.load for the entire queue. Critically, it assumed that if DV.viewers was available, then viewer.js had shown up and redefined DV.load as the true doc-loading function. So if DC.viewers was available but DV.load hadn't been redefined, then the loader's own DV.load would run instead, continuing to stuff docs in the queue every 500ms, doubling it indefinitely.

This is precisely what happened when the old oEmbed loader was paired with the new viewer.

New viewer.js renamed DV.load to DV.immediatelyLoadDocument, and then for compatibility with the old (non-oEmbed) loader aliased DV.load to DV.immediatelyLoadDocument but only if DV.load was undefined. Since DV.load was defined by the old oEmbed loader, the new viewer never aliased it, so it always referred to the queue-stuffing version defined in the old oEmbed loader.

Since we can't change the oEmbed loader – the whole problem is that it's cached inline in people's articles – our fix has to be to the viewer. Ideally, instead of just saying "is DV.load undefined?" we could say "is it undefined or was it defined by the old oEmbed loader"? Normally this would be a pipe dream, but we're saved by an unlikely rope: the old oEmbed loader uniquely sets DV._documentsWaitingForAppLoad = []. So! We can adapt the viewer to say "is DV.load undefined OR is DV._documentsWaitingForAppLoad defined?"

Performing that fix and continuing to test it now.

reefdog added a commit to documentcloud/document-viewer that referenced this issue Nov 3, 2016

@reefdog

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

Another broken scenario, though I haven't found this on any real sites: new loader + old viewer. This could happen if they were serving their own stale/cached version of the old viewer, but still pointing to our CDN for the loader. I have a plan to fix this.

reefdog added a commit that referenced this issue Nov 3, 2016

Update doc loader for compat with old viewers
* Fall back to old viewer’s `DV.load` if new viewer’s `DV.immediatelyLoadDocument` isn’t available
* Collapse `window.DV` to `DV` where possible
* Replace `querySelector` with `document.getElementsByTagName` for CSS/JS insertion sites

#420

reefdog added a commit to documentcloud/documentcloud-notes that referenced this issue Nov 4, 2016

reefdog added a commit that referenced this issue Nov 4, 2016

reefdog added a commit that referenced this issue Nov 4, 2016

@reefdog

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

New loaders fixed (presumably), tested, and deployed.

@reefdog reefdog closed this Nov 4, 2016

reefdog added a commit to documentcloud/documentcloud-notes that referenced this issue Feb 28, 2017

Squashed commit of the following:
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.