Currently we're using alert to show ajax errors. This is not a good way to deal with this.

We should be making our own Error type and throwing that to show when there is something that broke down. using alert or even console.log is a poor substitution when throw is really what we mean and want to do.

function AJAXError(message) {
  this.name = 'AJAXError';
  this.message = message || 'Default txt';
}
AJAXError.prototype = new Error();

This is the amount of code we need to throw proper errors that will show up with a stacktrace and everything we need to debug it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seutje’s picture

window.alert isn't a good way to deal with anything, except intentionally halting execution and possibly breaking a thing or 2 in the process.

Having real errors being thrown would not only keep certain interactions/animations from being halted (and evidentally breaking), but it would also allow us to make the error handling pluggable (e.g. a module could define an error handler that display it in a fancy modal, another could define a handler that posts it back to the server, ...)

markhalliwell’s picture

Awesome idea! I also agree, window.alert() is rather annoying.

klonos’s picture

Angry Dan’s picture

FileSize
2.81 KB

This patch replaces Drupal.ajaxError with an object that extends Error. I've also removed the usages of alert(Drupal.ajaxError()) and replaced them with thrown errors.

On it's own this would probably mean that there isn't enough feedback to the user in most cases, so I think we need to pursue some of the issues in #1419652: JavaScript logging and error reporting, specifically #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors before this can stand up.

Angry Dan’s picture

Status: Active » Needs review
nod_’s picture

Hey thanks for working on it :)

Any particular reason you went with using call on the constructor instead of prototypal inheritence? Typically I see throw new Error('my message'); when using Error, not so much this way of doing things.

Angry Dan’s picture

I must admit this is the first time that I've looked at JS for a couple few months now. I would like to see subtypes of the Error function for various errors in Drupal JS, just like you might want to do with the Exception class in PHP.

I was struggling to find the best way of subclassing Error. Are you suggesting that you wouldn't have done that and instead used descriptive error messages instead? I'm interested how would you have implemented this? I'm always looking for ways to improve!

nod_’s picture

Read the issue summary or look at drupal.js :) That's how I'd have done it.

(edit) and considering classes don't exist you can't really subclass it, you can inherit from it though.

attiks’s picture

#7 can you create a follow up issues for the subtypes?

nod_’s picture

I don't think we'd have that many, we can list them here.

there is DrupalBehaviorError already and we'd be creating a DrupalAjaxError for ajax things and maybe one other, not sure what for yet but I'm not seeing tens of error types defined by Drupal (well, I hope at least).

seutje’s picture

Removed some whitespace and added a semicolon that was removed.

Angry Dan’s picture

Hi all,

I haven't looked at this in a while now, but I thought I would link this article - https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Obj... which is where I got the methodology for extending Error in the way I did.

What else needs doing on this? I think it's probably ready to go in now, then we can move stuff around later/create new error types.

nod_’s picture

Removed the Error.call(), it's not needed, just need to set this.messages and it'll do the job.

Side effect of this patch is that there is no way to see an error if you don't have a JS console open somewhere. On way to address that is to use the future #77245: Provide a common API for displaying JavaScript messages. There is no way we'll be able to fix everything in one patch, this one is the first step to fix the really really annoying #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors issue.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Same code as #11 and that worked.

nod_’s picture

FileSize
690 bytes

interdiff

Angry Dan’s picture

Looks fine to me.

alexpott’s picture

Title: Use throw when error occurs » Use throw when an ajax error occurs and extend JavaScript's Error object

Improving title

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 1e40d90 and pushed to 8.x. Thanks!

webchick’s picture

What's the end-user experience for this now when a JS error is thrown? Will it silently fail unless you have a JS console open? If so, are we sure that's preferable?

attiks’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.