Add support for integrating with external reporting #196
Conversation
We use Raygun to log our javascript errors, and we'd like to automatically report migration warnings there too. Raising an event when a warning is logged seems a very natural way to do this - we can come scrape $.migrateWarnings when triggered. A stack trace also seems like a very useful thing to have, so extending the warning seems like a natural way to achieve this (it also helps a lot with Raygun, which likes to send Error objects).
Lint rules do not allow use of new String(), so use a parallel array to store Errors rather than a property on a String object.
Update tests to account for additional assert in expectWarning with a specified, non-zero count
|
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
|
What's the purpose of |
migrateWarnings.asErrors was supposed to wrap the message in an Error message to create a stack trace
|
@mgol Oops, I missed of wrapping it in a new Error() - added that now. |
|
I made the mistake of using my actual full name instead of my github username when signing the CLA. How do I fix those commits? |
You can rewrite b72995e with |
| }; | ||
|
|
||
| function migrateWarn( msg ) { | ||
| var console = window.console; | ||
| if ( !warnedAbout[ msg ] ) { | ||
| warnedAbout[ msg ] = true; | ||
| jQuery.migrateWarnings.push( msg ); | ||
| jQuery.migrateWarnings.asErrors.push( new Error( msg ) ); |
There was a problem hiding this comment.
I agree with @mgol; this is completely redundant.
There was a problem hiding this comment.
new Error( msg ).stack makes a difference (I managed to forget to put the wrapping in at first). We have a warning I'll have to try to track down after the weekend because we're muted for production and migrateWarnings doesn't provide context.
|
I think that the discussion in #15 still covers this use case, it doesn't seem to be a common enough need to add more plumbing. The warnings are already exposed as an array, you can enumerate them as needed and send them up to a server. If your error telemetry package already traps console messages it seems like you'd be getting this for free anyway. |
| if ( console && console.warn && !jQuery.migrateMute ) { | ||
| console.warn( "JQMIGRATE: " + msg ); | ||
| if ( jQuery.migrateTrace && console.trace ) { | ||
| console.trace(); | ||
| } | ||
| } | ||
| jQuery( window ).trigger( "migrateWarning" ); |
There was a problem hiding this comment.
The only time we currently call back into jQuery is to shim a method for warnings. This introduces a situation where we are using the host jQuery's functionality, which makes me nervous. It could potentially cause a recursive crash if Migrate was to give .trigger() warnings in the future.
There was a problem hiding this comment.
I'd consider it bad form to leak global events, but would be happy to see a hook that gets invoked here, including event emitter style (e.g., jQuery.migrateWarningHooks.push( handler ) or jQuery.migrateWarnings.on( "warning", handler )).
There was a problem hiding this comment.
I have to say I hadn't thought of intercepting the console. Obviously that would require unmuting jquery-migrate, and adding the wrapper in that intercept would be another uninformative line in the stack trace, but that's certainly no dealbreaker. Would scraping for /^JQMIGRATE:/ be safe both now and in the future for consumers who have other cruft in their console they aren't interested in recording?
I understand your hesitation about looping back through .trigger, although it would certainly be possible if you were duckpunching trigger to call the original directly in that one case (or presumably if you were raising warnings, there's a decent chance it'd be because some other way would be preferred).
There was a problem hiding this comment.
I have to say I hadn't thought of intercepting the console. Obviously that would require unmuting jquery-migrate, and adding the wrapper in that intercept would be another uninformative line in the stack trace
Then duckpunch jQuery.migrateWarnings.push like I suggested in #15.
We use Raygun to record JavaScript errors. It would be nice to have migrate warnings there too. Raygun likes Error objects - and we like stack traces - so save Error objects in parallel with the warning messages in migrateWarn() and also raise an event that a listener can trigger on to extract the data & send it wherever it wishes.