Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Load embed JS async and without document.write #418

Merged
merged 12 commits into from
Nov 1, 2016
Merged

Conversation

reefdog
Copy link
Contributor

@reefdog reefdog commented Oct 31, 2016

Copying and genericizing the description of this pull request.

Our document, note, and search embed codes all follow this pattern (using pseudo-code):

<div id="foo-1234"></div>
<script src="loader.js"></script>
<script>
  loadEmbed('//www.documentcloud.org/path/to/foo-1234.json');
</script>

So, our current codes depend on render-blocking synchronous script loading:

  1. loader.js loads remotely (blocking)
  2. loader.js uses document.write to add resource-specific embed.js to the DOM (bad practice and being deprecated)
  3. embed.js loads remotely (blocking)
  4. loadEmbed 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.

What this PR will do

  1. Define the inline embed-loading functions (loadEmbed in the pseudo-code up there) as queuing functions in the loader JS, unless the embed JS is already available, then skip the queue and load immediately
  2. Have the loader JS use document.createElement (instead of document.write) to add the embed JS to the DOM, and only if necessary, and using async so page render can continue
  3. Rename the curerent embed-loading functions in the embed JS (dc.embed.loadNote/dc.embed.load/DV.load) for the loader JS to call once the embed JS is available, but also alias the old name to the new name when the old name isn't defined (for scenarios where the embed JS is used without the loader JS)

One downside: we now require IE8+ IE9+.

What this PR won't do

  1. Change the embed codes: Since we need to maintain compatibility with the existing embed codes anyway, this work doesn't touch them.
  2. Make our embeds work better in pjaxy environments: See this comment.
  3. Unify embed loaders: This is a pet project of mine, and won't be hard to do, but it's just outside the scope of this PR. So despite heavy duplication across the various loaders, this doesn't alleviate that yet.

Status:

  • Note embeds
  • Document embeds
  • Search embeds

@knowtheory
Copy link
Member

we now require IE8+.

That's fine.

@reefdog
Copy link
Contributor Author

reefdog commented Oct 31, 2016

Gah, I actually meant IE9+, both here and in the other PR, but it seems you read my mind anyway and still agree.

Again, I can work around that and support IE8 (and maybe earlier) without much trouble. It'll just be a bit more code in the loader.

reefdog added a commit to documentcloud/document-viewer that referenced this pull request Oct 31, 2016
As of documentcloud/documentcloud#418, the individual embed loaders will be redefining their public entry points as queuing functions so that the actual embed libraries (like document_viewer.js) can be loaded asynchronously.

- Rename public entry function from `DV.load` to `DV.privateLoadDocument`
- Alias `DV.load` to `DV.privateLoadDocument` when `DV.load` is undefined (for use without the loader or with stale versions of the loader)

Affects #58
- Adapt async note embed loader for use by document embed
- Remove redundant oembed loader (since this one queues now too)

See #418 and documentcloud/document-viewer#58
@reefdog
Copy link
Contributor Author

reefdog commented Oct 31, 2016

Need to put this somewhere, may as well be here: if we ever wanted to ensure IE8 compatibility, we'd have to use the following addEventListener polyfill:

var on = function (el, eventName, handler) {
  if (el.addEventListener) {
    el.addEventListener(eventName, handler);
  } else {
    if (el.tagName.toLowerCase() == 'script' && eventName.toLowerCase() == 'load') {
      el.attachEvent('onreadystatechange', function() {
        if (el.readyState == 'complete' || el.readyState == 'loaded') {
          handler.call(el);
        }
      });
    } else {
      el.attachEvent('on' + eventName, function() {
        handler.call(el);
      });
    }
  }
};

And replace the $= attribute matcher with just = in the querySelectors.

The `attachEvent` polyfill failed on script insertions in IE8 anyway (see #418 (comment)), so unless people clamor for IE8 support, go all-native (but check for support so as not to throw errors).
Before doing the work necessary for #418, build search_embed.js from latest dependencies.
@knowtheory
Copy link
Member

Just to be definitive, have you pulled the IE numbers from Google Analytics?

@knowtheory
Copy link
Member

I'll take a look at pulling the last month's worth of raw logs.

@reefdog
Copy link
Contributor Author

reefdog commented Nov 1, 2016

In October 2016, IE6-IE8 combined were 0.2% of our total website/platform visits. That doesn't track embeds. For YTD 2016, IE6-IE8 were 0.43%.

If we can rely on IE9+, we don’t need to feed these non-dataURI CSS files.
This is now written without document.write by the embed loader itself
@reefdog reefdog merged commit 1b5e869 into master Nov 1, 2016
@reefdog
Copy link
Contributor Author

reefdog commented Nov 1, 2016

Got the LGTM from @knowtheory via Slack. Away we go!

@reefdog reefdog deleted the async-loaders branch November 1, 2016 22:27
reefdog added a commit that referenced this pull request Nov 1, 2016
Deployed #418, immediately saw sites breaking. Rescued it by finding and deploying the old loaders to S3. Storing them here so we have access until we know what’s up.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants