Skip to content

Add default exception handler to Ajax.Base #302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Add default exception handler to Ajax.Base #302

wants to merge 1 commit into from

Conversation

Gargaj
Copy link

@Gargaj Gargaj commented Sep 28, 2015

If this isn't specified, any exception that is generated inside an Ajax call is swallowed, which I think is mostly an undesired effect since execution just stops without any feedback in e.g. the console or the user's outer exception handler.

If this isn't specified, any exception that is generated inside an Ajax call is swallowed, which I think is mostly an undesired effect since execution just stops without any feedback in e.g. the console or the user's outer exception handler.
@Gargaj Gargaj changed the title Add default exception handler to Ajax base Add default exception handler to Ajax.Base Sep 28, 2015
@savetheclocktower
Copy link
Collaborator

Marking this as a bug, but I'll have to decide on how best to handle this. Adopting this PR would mean a change to existing default behavior.

@Gargaj
Copy link
Author

Gargaj commented Jan 3, 2016

I would argue that any site that relies on a framework to swallow exceptions should be forced to do it manually. If your site is continually throwing exceptions, the framework shouldn't be holding them back - relying on it is just bad behaviour that should not be condoned.

If anything, having this in a future version's patch notes would encourage site owners to go through their existing Ajax and check if their code is solid when it comes to this.

@Gargaj
Copy link
Author

Gargaj commented Mar 3, 2017

@savetheclocktower Can has 1.7.4 plees? See my argument above about why this would be necessary for a better web.

@savetheclocktower
Copy link
Collaborator

How about this as the default handler:

function(request, exception) {
  setTimeout(function() { throw exception; }, 0);
}

My concern is that if anyone is relying on current behavior, upgrading will cause their code execution to halt in places it didn't before. At least this way the only behavior change is that exceptions would show up in the console where they didn't before.

@savetheclocktower savetheclocktower added this to the 1.8.0 milestone Apr 8, 2017
@Gargaj
Copy link
Author

Gargaj commented Apr 9, 2017

If they're relying on current behaviour, their code will halt anyway, won't it? Just because they don't see the Exception doesn't mean it doesn't fire and kill execution.

@jwestbrook
Copy link
Collaborator

Idea : instead of throwing exception - use console.warn or console.error if it exists? It keeps the current behavior but notifies the dev that something went wrong

@savetheclocktower
Copy link
Collaborator

My point is that an asynchronous throw will halt only the stuff that’s happening in that scheduled unit. But then I suppose my suggestion is only an improvement for synchronous requests, which are dumb anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants