Skip to content

Deferred: Respect source maps in jQuery.Deferred.exceptionHook #5192

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.

Sign up for GitHub

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

Jump to bottom
Merged
mgol merged 1 commit into from
Feb 1, 2023
Merged

Deferred: Respect source maps in jQuery.Deferred.exceptionHook #5192

mgol merged 1 commit into from
Feb 1, 2023

Conversation

@mgol
Copy link
Member

@mgol mgol commented Jan 12, 2023 •
edited
Loading

Summary

So far, jQuery.Deferred.exceptionHook used to log error message and stack separately. However, that breaks browser applying source maps against the stack trace - most browsers require logging an error instance. This change makes us do exactly that.

One drawback of the change is that in IE 11 previously stack was printed directly and now just the error summary; to get to the actual stack trace, three clicks are required. This seems to be a low price to pay for having source maps work in all the other browsers, though.

Safari with the new change requires one click to get to the stack trace which sounds manageable.

Fixes gh-3179
Ref https://crbug.com/622227

-12 bytes

I'd like to keep it a 4.x item only, for two reasons:

  • this may be an edge case but some integrations may depend on our console.warn pattern and changing it would break them
  • it degrades the debugging experience for all IE (especially 9-10 that have terrible DevTools) & Edge Legacy versions that we still support in v3; I also haven't tested other older browsers that we still test against with 3.x
  • this improvement doesn't apply to running apps but to development only so I don't feel like it necessarily needs to make it to 3.x

I used a modified test case from gh-3179:

c = ->
    foobar()

b = ->
    c()

a = ->
    b()

window.addEventListener 'DOMContentLoaded', ->
    console.log 'from dom content loaded'
    a()

$ ->
    console.log 'from jQuery ready'
    a()

$.when().then ->
    console.log 'from jQuery Deferred'
    a()

Screenshots comparing before & after below. One error coming from jQuery is available even in the "before" case as we still have jQuery.readyException that re-throws errors from jQuery ready.

Chrome

Before

jq-source-maps-chrome-jq3

After

jq-source-maps-chrome-jq4

Firefox

Before

jq-source-maps-firefox-jq3

After

jq-source-maps-firefox-jq4

Safari

Before

jq-source-maps-safari-jq3

After

Initial view

jq-source-maps-safari-jq4-1

After a single click on each exception

jq-source-maps-safari-jq4-2

IE 11

Before

jq-source-maps-ie11-jq3

After

Initial view

jq-source-maps-ie11-jq4-1

After the 1st click

jq-source-maps-ie11-jq4-2

After the 2nd click

jq-source-maps-ie11-jq4-3

After the 3rd click

jq-source-maps-ie11-jq4-4

Checklist

@mgol
Deferred: Respect source maps in jQuery.Deferred.exceptionHook
b418fc2 
So far, `jQuery.Deferred.exceptionHook` used to log error message and stack
separately. However, that breaks browser applying source maps against the stack
trace - most browsers require logging an error instance. This change makes us
do exactly that.

One drawback of the change is that in IE 11 previously stack was printed
directly and now just the error summary; to get to the actual stack
trace, three clicks are required. This seems to be a low price to pay
for having source maps work in all the other browsers, though.

Safari with the new change requires one click to get to the stack trace which
sounds manageable.

Fixes jquerygh-3179
Ref https://crbug.com/622227
@mgol mgol added Deferred Needs review Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. labels Jan 12, 2023
@mgol mgol added this to the 4.0.0 milestone Jan 12, 2023
@mgol mgol self-assigned this Jan 12, 2023
@mgol mgol force-pushed the exceptionHook-stack-trace branch from 42f40e5 to b418fc2 Compare January 12, 2023 14:18
@mgol mgol mentioned this pull request Jan 12, 2023
Closed
@mgol
Copy link
Member Author

mgol commented Jan 12, 2023

I know this doesn't solve ALL the issues outlined in #3179 (in particular, it doesn't address issues tackled by @dmethvin's https://github.com/dmethvin/jquery-deferred-reporter) but at least the source is mapped & formatted by the browser properly now, you can click to the source and the error is highlighted there.

mgol
mgol commented Jan 12, 2023
View reviewed changes
src/deferred/exceptionHook.js
error.stack,
"jQuery.Deferred exception",
error,
stack
Copy link
Member Author

@mgol mgol Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about the name of this argument. The idea was to fill it in via https://github.com/dmethvin/jquery-deferred-reporter or something equivalent but that would hit the same issue with unmapped source maps for that additional stack. Should we assume that we should also pass a full error object here instead?

@dmethvin thoughts?

Copy link
Member Author

@mgol mgol Jan 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One difficulty is the name of the hook is jQuery.Deferred.getStackHook so it explicitly talks about stack, not an error...

Copy link
Member

@dmethvin dmethvin Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could introduce a new jQuery.Deferred.getErrorHook perhaps and deprecate the old hook?

Copy link
Member Author

@mgol mgol Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! I'll do this separately. This will mostly just be a rename as we'll be using it in the same way, just the recommendation will change.

Copy link
Member Author

@mgol mgol Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted #5201 to track this.

@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Jan 30, 2023
timmywil
timmywil approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@timmywil timmywil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this! Makes sense to just log the error object.

dmethvin
dmethvin approved these changes Jan 31, 2023
View reviewed changes
Copy link
Member

@dmethvin dmethvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been forever since I looked at the way browsers handled stack traces in the console, especially for Promises. I think they've gotten a lot better, for example V8 enabled async stack traces in 2019. So this PR should improve things.

@mgol mgol mentioned this pull request Feb 1, 2023
Closed
@mgol mgol merged commit 0b9c503 into jquery:main Feb 1, 2023
@mgol mgol removed the Needs review label Feb 1, 2023
@mgol mgol deleted the exceptionHook-stack-trace branch February 1, 2023 12:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dmethvin dmethvin dmethvin approved these changes

@timmywil timmywil timmywil approved these changes

Assignees

@mgol mgol

Labels

Deferred

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

jQuery.Deferred.exceptionHook breaks sourceMaps

3 participants

@mgol @dmethvin @timmywil