Open westonruter opened 5 years ago
Thanks for raising this issue, @westonruter. cc'ing in @lukeed in case he's interested in the discussion.
quicklink
isn't currently setup to be loaded asynchronously in the same way AMP. This is because we can't guarantee the library has been loaded when a user calls the quicklink()
method. I've attempted a few variations of the patterns you suggested but don't believe we can make it work without changes to core.
Option 1
If quicklink()
automatically instantiates when loaded (without requiring a user to call quicklink()
...) we wouldn't have problems with folks loading the script async
. The library would handle event listening and trigger whenever it has been fetched. The big problem here is lack of configuration, or users needing to define their config separately (e.g define window.quicklinkConfig = {...}
early on) and have quicklink()
read this whenever it has finished loading. Not the end of the world, but the DX is a little... messy :)
Option 2
An alternative is suggesting some variation of a simple script loader / import that loads quicklink
async and then instantiates whenever that has finished resolving. It's been a while since I've dealt with async script loading patterns so I'm very open to hearing other ideas we could investigate here.
const url = 'quicklink.umd.js';
function loadScript(src) {
return new Promise((resolve, reject) => {
var s;
s = document.createElement('script');
s.src = src;
s.async = true;
s.onload = resolve;
s.onerror = reject;
document.head.appendChild(s);
});
}
loadScript(url)
.then(()=> {
quicklink();
});
The way I've handled similar things like this in the past is thru something like this:
function myInit() {
// ... everything else
quicklink();
}
// If the page was already loaded/ready by the time
// we've arrived here, call the init function. Otherwise,
// we add an event listener for the initializer
/complete|interactive/.test(document.readyState)
? myInit() : addEventListener('DOMContentLoaded', myInit);
Not sure if this should be in core, but it definitely could be. I'm only hesitant because I think quicklink is used mostly in UI libs/frameworks where some level of DOM-readiness is already guaranteed.
quicklink isn't currently setup to be loaded asynchronously in the same way AMP. This is because we can't guarantee the library has been loaded when a user calls the
quicklink()
method.
I'm not sure about the need to guarantee that the library has been loaded. Isn't that the point of defining quicklink
as a global array which is then overridden once Quicklink is loaded?
Given this code I shared above:
(window.quicklink = window.quicklink || []).push(function(quicklink) {
quicklink();
});
Couldn't Quicklink be set up to follow the same pattern by starting out with something like:
// Capture any callbacks that were registered prior to loading quicklink.js
const callbacks = [];
if ( Array.isArray( window.quicklink ) ) {
callbacks.push( ...window.quicklink );
}
// Override any global quicklink array with the quicklink function.
window.quicklink = function( options ) {
// ...
};
// Now augment the quicklink function with a push method which is just an alias to invoking itself.
/**
* Invoke callback immediately once quicklink is loaded.
*
* @param {Function} callback
*/
window.quicklink.push = function ( callback ) {
callback( this );
}
// Invoke callbacks that were registered prior to loading quicklink.js.
callbacks.forEach( ( callback ) => {
callback( window.quicklink );
} );
We have this in inside an external async script, conditionally, and it works fine:
const script = document.createElement("script");
script.src = "/js/quicklink.umd.min.js";
script.addEventListener("load", () => {
// eslint-disable-next-line no-undef
quicklink.listen();
});
document.body.append(script);
So quicklink.umd.min.js
is not async itself, but is added by an async script, which I understand accomplishes the same.
Currently the readme suggests to include the script into the page via:
or at
load
event via:However, shouldn't the
quicklink.umd.js
script be loaded withasync
to begin with? The blocking script is not good for performance.Ideally you should be able to do something like this, similar to the AMP shadow doc API: