Idempotent JavaScript script loader

Question

I wrote an idempotent script loader (for the browser), where the same script won't be loaded more than once.

scriptLoader.js:

let _loadingAndLoadedScripts = [];
export async function loadJs(src) {
 let hash = generateHash(src);
 // scenario 1: first invocation, so load script
 if (!_loadingAndLoadedScripts[hash]) {
 return new Promise((resolve, reject) => {
 let tag = document.createElement('script');
 tag.type = 'text/javascript';
 tag.id = hash;
 tag.src = src;
 tag.onload = () => {
 tag.setAttribute('data-loaded', true);
 document.dispatchEvent(new Event('scriptLoaded'));
 resolve();
 };
 tag.onerror = () => {
 console.error('Failed to load script \'' + src + '\'.');
 reject();
 }
 _loadingAndLoadedScripts[src] = true; // save state
 document.body.appendChild(tag);
 });
 }
 // scenario 2: script is busy loading, or already loaded
 else {
 // if loaded, do nothing
 var script = document.getElementById(hash);
 let isLoaded = script && script.getAttribute('data-loaded') === 'true';
 if (isLoaded) {
 return Promise.resolve();
 }
 // else loading, so wait
 else {
 return new Promise((resolve, reject) => {
 // ------------------ MAYBE WRAP IN TIMEOUT?
 document.addEventListener('scriptLoaded', (e) => {
 resolve();
 }, { 'once': true });
 // ------------------ MAYBE WRAP IN TIMEOUT?
 });
 }
 }
}
function generateHash(s) {
 // implementation irrelevant
}

It works.

But my JS is a bit rusty.I was thinking there could be a race condition whereby invocations 2+ (which wait for the script to load) would not register an event listener in time, and so wait forever for the event. A solution is to add a timer, and if it expires to reject the promise. I've shown that in the code above as // MAYBE WRAP IN TIMEOUT?.

This is the implementation I want to use inside of //----- ... //-----:

return new Promise((resolve, reject) => {
 let timer;
 let eventName = 'scriptLoaded';
 let eventHandler = (event) => {
 clearTimeout(timer);
 resolve();
 }
 let eventOptions = { 'once': true };
 document.addEventListener(eventName, eventHandler, eventOptions);
 timer = setTimeout(() => {
 document.removeEventListener(eventName, eventHandler, eventOptions);
 console.error('Failed to load script \'' + src + '\'.');
 reject();
 }, 10 * 1000);
});

It works.

But I'm wondering whether it's overkill and/or unnecessary? JS in the browser is single-threaded, so is it even possible that such a race condition would occur?

Answer 1

A few issues

Needless use of async function

If you are not going to use the await keyword then a normal function would be sufficient.

Use dataset property instead of setAttribute with data attributes.

So instead of tag.setAttribute('data-loaded', true); you can do tag.dataset.dataLoaded = true;. You may not even need to use this attribute at all.

Use Object or Map instead of Array for hash tables

Assuming your hash is a string like "2CF24...8B9824" then you should at least use an object for _loadingAndLoadedScripts. Let's use a Map object this time.

data-loaded attribute is unnecessary.

Since we will be using the returned promise to decide whether the script tag is loaded or not this is redundant.

OK now in your code the loadJS function returns a promise but you never use it. In fact that promise is exactly the only thing that you need. Accordingly we can trash that scriptLoaded event.

function generateHash(s) {
 // implementation irrelevant
}
export function loadJS(src, hash) {
 return new Promise((resolve, reject) => {
 let tag = document.createElement('script');
 tag.type = 'text/javascript';
 tag.id = hash;
 tag.src = src;
 tag.onload = resolve;
 tag.onerror = e => {
 console.error('Failed to load script \'' + src + '\'.');
 tag.remove();
 reject({error:e,hash,src});
 }
 document.body.appendChild(tag);
 });
}
let _loadingAndLoadedScripts = new Map(),
 src = "https://cdnjs.cloudflare.com/ajax/libs/ace/1.15.0/ace.js",
 hash = generateHash(src);
if (!_loadingAndLoadedScripts.has(hash)) _loadingAndLoadedScripts.set(hash,loadJS(src,hash));
// if you want the promise way
_loadingAndLoadedScripts.get(hash)
 .then(_ => { /* proceed with whatever code here */ })
 .catch(e => ( _loadingAndLoadedScripts.delete(e.hash)
 , doSomethingWith(e.src,e.hash)
 ));
// or if you have top level await
try {
 await _loadingAndLoadedScripts.get(hash);
 // proceed with whatever code here 
} catch (e) {
 _loadingAndLoadedScripts.delete(e.hash);
 doSomeThingWith(e.src,e.hash);
}

We start with a source url as src and calculate it's hash with generateHash function then we pass them to the loadJS function which returns a promise which in return stored in the hash map _loadingAndLoadedScripts as [hash, promise] key value pairs.

So any time you have a source to create a new script tag to avoide a duplicate first check the hash table with the source string's hash. If it returns a promise then it's either already resolved (script loaded) or waiting. Whatever it's state just await or .then() it and proceed with the next source.

This workflow guarantees that the scripts are loaded in the order you invoke loadJS. You may add a timeout functionality to the loadJS file which rejects the promise once a certain time limit is reached. In this case you should remember that the hash table for that source has already been set so just remove the etry with that hash from the hash table.

Answer 2

scriptLoaded event may be triggered by another script

1. Loading scriptA
2. Loading scriptB
3. Loading scriptB again
 3.1 waiting for scriptLoaded event
4. Loaded scriptA
 4.1 scriptLoaded event fired
 4.2 the second scriptB loading report finished incorrectly

Cache the Promise instead could be another solution

if (script_url not in promiseCache) {
 promise = new Promise((resolve, reject) => {
 more stuff about loading script
 });
 promiseCache[script_url] = promise;
}
return promiseCache[script_url];

So you can avoid the event here.

• lonix: Thanks tsh! Event may be triggered by other script: very nice catch, didn't think of that. I could fix it by raising it on a specific tag instead. Caching promise: which part of the code does that replace?