patternjavascriptMinor

Loading the resource identified by a script element's src attribute

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

scripttheresourcesrcloadingelementattributeidentified

Problem

I have a function to load the resource identified by a ` element's src

 attribute.

I need it to:

Be as universal as possible.

Work on file-to-file requests (from file:// to file://), if the browser allows to.




This is the code I have so far:

var asyncGetContents = function(node,listener){
   var hasRun = false;
   if(node.src){
      var httpRequest = new XMLHttpRequest();
      httpRequest.onreadystatechange = function() {
         if(httpRequest.responseText){
            if(hasRun){return;}
            hasRun = true;
            listener(httpRequest.responseText);
            httpRequest.onreadystatechange=undefined;
         }
      };
      httpRequest.open('GET', node.src);
      if(node.type){
         httpRequest.overrideMimeType(node.type);
      }
      httpRequest.send();
   }else{
      listener(node.textContent);
   }
};


Is this code well-formed?



Are there any aspects that I could've possibly missed?

Is there any way to do it without the variable hasRun? Removing the onReadyStateChange event listener doesn't seem to work - listener` function runs twice.

Solution

The code looks well-formed. I'd like some comments though and maybe a bit of whitespace here and there.

However, I think there's a good chance this will actually fail. readystatechange fires several times during the lifetime of an XHR object. But your code doesn't actually check the XHR object's readyState, which is what the event is there to tell you about.

Instead it checks responseText, but if the file is large enough to fire several events during its download, responseText may only hold partial data. So responseText is truthy, and hasRun is set to true, but the code won't ever get the complete data.

There's are 5 distinct readyState values. You should be checking for the DONE state only.

Alternatively, for some (most, I believe) browsers you can simply hook your functions to onload, onerror and friends instead of of the general onreadystatechange.

You may also want to check the arguments: Does node exist? Is listener a function?

Lastly, how do you handle failures? A 404 will still give you responseText, just not anything useful. A simple solution would be something like Node.js's callback pattern, of having the listener receive 2 arguments: err and data. If err is truthy, something went wrong. You also want to simply pass the XHR object along as the 3rd argument, so the listener can make whatever checks it wants to (much like jQuery does).

function asyncGetContents(node, listener) {
  var xhr;

  // Example of handling a bad node argument
  // Adapt it to fit your needs
  if(!node) {
    throw new TypeError;
  }

  // Example of handling a bad listener argument
  // Adapt it to fit your needs
  if(typeof listener !== 'function') {
    throw new TypeError;
  }

  if(node.src) {
    xhr = new XMLHttpRequest();
    xhr.onreadystatechange = function () {
      // check readyState
      if(xhr.readyState === XMLHttpRequest.DONE) {
        // check that the response code is 2xx
        if(xhr.status >= 200 && xhr.status < 300) {
          listener(null, xhr.responseText, xhr);
        } else {
          listener(xhr.status, null, xhr); // Provide a more useful error here
        }
      }
    };

    xhr.open('GET', node.src);
    if(node.type) {
      xhr.overrideMimeType(node.type);
    }
    xhr.send();
  } else {
    // we've only checked that node is truthy, not that it's
    // actually a script element, so node.textContent may
    // be `undefined`, but let's leave that to the listener
    listener(null, node.textContent, null);
  }
}

Code Snippets

function asyncGetContents(node, listener) {
  var xhr;

  // Example of handling a bad node argument
  // Adapt it to fit your needs
  if(!node) {
    throw new TypeError;
  }

  // Example of handling a bad listener argument
  // Adapt it to fit your needs
  if(typeof listener !== 'function') {
    throw new TypeError;
  }

  if(node.src) {
    xhr = new XMLHttpRequest();
    xhr.onreadystatechange = function () {
      // check readyState
      if(xhr.readyState === XMLHttpRequest.DONE) {
        // check that the response code is 2xx
        if(xhr.status >= 200 && xhr.status < 300) {
          listener(null, xhr.responseText, xhr);
        } else {
          listener(xhr.status, null, xhr); // Provide a more useful error here
        }
      }
    };

    xhr.open('GET', node.src);
    if(node.type) {
      xhr.overrideMimeType(node.type);
    }
    xhr.send();
  } else {
    // we've only checked that node is truthy, not that it's
    // actually a script element, so node.textContent may
    // be `undefined`, but let's leave that to the listener
    listener(null, node.textContent, null);
  }
}

Context

StackExchange Code Review Q#54293, answer score: 2

Revisions (0)

No revisions yet.