Return to Answer

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).

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).

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. Since you just want to merge the given options without sanitizing etc., just reassign to options.

• 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).

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).