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.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | interdiff.txt | 690 bytes | nod_ |
| #13 | core-js-throw-ajaxerror-1606362-13.patch | 2.92 KB | nod_ |
| #11 | ajax-error-1606362-11.patch | 2.81 KB | seutje |
| #11 | ajax-error-1606362-11-interdiff.txt | 1016 bytes | seutje |
| #4 | ajax-error-1606362-4.patch | 2.81 KB | Angry Dan |
Comments
Comment #1
seutje commentedwindow.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, ...)
Comment #2
markhalliwellAwesome idea! I also agree, window.alert() is rather annoying.
Comment #3
klonos...coming from #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors
Comment #4
Angry Dan commentedThis 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.
Comment #5
Angry Dan commentedComment #6
nod_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.Comment #7
Angry Dan commentedI 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!
Comment #8
nod_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.
Comment #9
attiks commented#7 can you create a follow up issues for the subtypes?
Comment #10
nod_I don't think we'd have that many, we can list them here.
there is
DrupalBehaviorErroralready and we'd be creating aDrupalAjaxErrorfor 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).Comment #11
seutje commentedRemoved some whitespace and added a semicolon that was removed.
Comment #12
Angry Dan commentedHi 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.
Comment #13
nod_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.
Comment #14
nod_Same code as #11 and that worked.
Comment #15
nod_interdiff
Comment #16
Angry Dan commentedLooks fine to me.
Comment #17
alexpottImproving title
Comment #18
alexpottCommitted 1e40d90 and pushed to 8.x. Thanks!
Comment #19
webchickWhat'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?
Comment #20
attiks commentedDoes this needs backporting to D7, or do we wait till #1232416: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors is fixed?