Get Ajax request implementation without jQuery

Question

I'm writing a pixel library to do an AJAX request so I'm not allowed to use any helper.

function ajax(options) {
 var xhr = new XMLHttpRequest();
 var opts = Object.assign({
 withCredentials: false,
 method: 'GET'
 }, options);
 xhr.withCredentials = options.withCredentials;
 xhr.open(opts.method, opts.url);
 xhr.setRequestHeader('Accept', 'text/plain');
 xhr.send(null);
 return {
 done: function(cb) {
 xhr.onreadystatechange = function onStateChange() {
 if (this.readyState === 4) {
 if (this.status >= 200 && this.status < 300) {
 cb(this.responseText);
 } else {
 cb('error');
 }
 }
 };
 xhr.ontimeout = function(e) {
 console.error(e);
 cb(false);
 };
 xhr.onerror = function(e) {
 console.error(e);
 cb(false);
 };
 }
 }
};
ajax({
 url: '/echo/json'
}).done(function(response) {
 console.log(response);
})

I also have my code in a jsfiddle in case anyone wants to test the code.

Note: This library doesn't support POST as I want to keep the code as less as possible. It will only support GET.

Answer 1

Your ajax() function gives me a good first impression. The structure seems sound and the event handling well done - the done() event handler is called exactly once (someone else might want to confirm, though).

A few suggestions:

• var opts = Object.assign({...}, options);

  opts vs. options is inconsistent and carries no semantics. Reassigning to options would mutate the argument which could cause issues for the user. jQuery names the merged options and defaults settings, which might be a little bit more clear.

• if (this.status >= 200 && this.status < 300) {

  This line seems good for now. You might want to check 304 in a future version when allowing users to supply caching headers. However, such requests don't respond with data and are probably not within the intended use of your library right now.

• xhr.send(null);

  The send method accepts an optional parameter which is ignored and set to null for GET requests. Therefore, I would recommend the simpler xhr.send().

• if (this.readyState === 4) {

  Add more semantic by replacing 4 with XMLHttpRequest.DONE - not available in old browsers such as IE 8.

• cb('error');

  This seems like a design flaw to me. A user cannot distinguish between an 'error' string received from the server vs. supplied as a result of an invalid HTTP status code. You already supply false on multiple occasions - when a network level error or timeout occurs - which is easily distinguished from a valid response by type checking. Why not supply false in the above case, too? If that's not an option, supply an error status flag or object or introduce error callback(s).

Answer 2

I updated your code a little bit. Have a nice day ;)

[UPDATE]:

I have used arrow functions to write cleaner and shorter code. You can see it for example at the end of the ajax function (ontimeout and onerror)

before:

 xhr.ontimeout = function(e) {
 console.error(e);
 cb(false);
 };

after:

 xhr.ontimeout = e => reject(e);

Also i've used the spread operator to extend the opts variable. Info: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Operators/Spread_operator

ajax, xhr and opts changed from var to const, because theire values never change.

The ajax function now returns a Promise instead of calling a function. Better error handling, chaining and state of the art.

I've removed all console.log calls and always return an Error Object for better handling instead of false.

---

const ajax = (options) => {
 const xhr = new XMLHttpRequest();
 // request options
 const opts = {
 withCredentials: false,
 method: options.method || 'GET',
 ...options
 };
 xhr.withCredentials = options.withCredentials;
 xhr.open(opts.method, opts.url);
 xhr.setRequestHeader('Accept', opts.type || 'text/plain');
 xhr.send(opts.body || null);
 // create promise to handle asynchron call
 return new Promise((resolve, reject) => {
 xhr.onreadystatechange = () => {
 if (xhr.readyState === XMLHttpRequest.DONE) {
 if (xhr.status >= 200 && xhr.status < 300) {
 resolve(xhr.responseText);
 } else {
 // return errors if status is wrong (catch)
 reject(new Error(xhr.status));
 }
 }
 }
 // return errors on timeout (catch)
 xhr.ontimeout = e => reject(e);
 xhr.onerror = e => reject(e);
 })
}
ajax({
 url: '/echo/json',
 method: 'POST', // Standard: GET
 type: 'application/json', // Standard: text/plain
 body: { // Standard: null
 form: 'data'
 }
}).then(data => data)
 // chaining. Handle returned `data` in next `then`
 .then((data) => {
 console.log(data);
}).catch((error) => {
 console.log(error);
});

• le_m: Very nice! Don't mind me suggesting a few additional ideas: The default options + options merging could be a bit more localized instead of all over the place. opts.body || null might set an incorrect null POST body for e.g. opts.body = 0. Also, the callback receives an Error when an application layer error occurs (HTTP status code) but a ProgressEvent when a transport layer error occurs (xhr.onerror) which seems a bit quirky.
• Mike Brant: Just rewriting code without explaining what you rewrote and why does not really give much value to either the original question poster or to others reading the answer. Therefore it doesn't really amount to a code review at all. Can you elaborate here?
• Philip: Thanks for your comment. You're right. I am new to this and learning with every post. I've updated my answer.