Skip to content

Add support for integrating with external reporting #196

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

Closed
wants to merge 6 commits into from
Closed

Add support for integrating with external reporting #196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c89540f
Simplify integration with reporting tools
sullivanst May 27, 2016
8698dd6
Make it lint
sullivanst May 27, 2016
6f78e58
Fix spacing for lint style
sullivanst May 27, 2016
02c4f38
Add check for warning as Errors
sullivanst May 27, 2016
b72995e
Tests: get passing
mattwTS May 27, 2016
97eddc0
Add missing Error wrapper
sullivanst May 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/migrate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ var warnedAbout = {};

// List of warnings already given; public read only
jQuery.migrateWarnings = [];
jQuery.migrateWarnings.asErrors = [];

// Set to false to disable traces that appear with warnings
if ( jQuery.migrateTrace === undefined ) {
Expand All @@ -40,20 +41,22 @@ if ( jQuery.migrateTrace === undefined ) {
// Forget any warnings we've already given; public
jQuery.migrateReset = function() {
warnedAbout = {};
jQuery.migrateWarnings.length = 0;
jQuery.migrateWarnings.length = jQuery.migrateWarnings.asErrors.length = 0;
};

function migrateWarn( msg ) {
var console = window.console;
if ( !warnedAbout[ msg ] ) {
warnedAbout[ msg ] = true;
jQuery.migrateWarnings.push( msg );
jQuery.migrateWarnings.asErrors.push( new Error( msg ) );
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @mgol; this is completely redundant.

Copy link
Author

Choose a reason for hiding this comment

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

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.

if ( console && console.warn && !jQuery.migrateMute ) {
console.warn( "JQMIGRATE: " + msg );
if ( jQuery.migrateTrace && console.trace ) {
console.trace();
}
}
jQuery( window ).trigger( "migrateWarning" );
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 )).

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

That'll work.

}
}

Expand Down
2 changes: 1 addition & 1 deletion test/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
module( "ajax" );

test( "jQuery.ajax() deprecations on jqXHR", function( assert ) {
assert.expect( 3 );
assert.expect( 4 );

var done = assert.async();

Expand Down
4 changes: 2 additions & 2 deletions test/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
QUnit.module( "attributes" );

QUnit.test( ".removeAttr( boolean attribute )", function( assert ) {
assert.expect( 8 );
assert.expect( 9 );

expectNoWarning( "non-boolean attr", function() {
var $div = jQuery( "<div />" )
Expand Down Expand Up @@ -43,7 +43,7 @@ QUnit.test( ".removeAttr( boolean attribute )", function( assert ) {
} );

QUnit.test( ".toggleClass( boolean )", function( assert ) {
assert.expect( 14 );
assert.expect( 17 );

var e = jQuery( "<div />" ).appendTo( "#qunit-fixture" );

Expand Down
2 changes: 1 addition & 1 deletion test/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ test( "jQuery( '#' )", function() {
} );

QUnit.test( "Attribute selectors with unquoted hashes", function( assert ) {
expect( 31 );
expect( 34 );

var markup = jQuery(
"<div>" +
Expand Down
2 changes: 1 addition & 1 deletion test/data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ test( "jQuery.data() camelCased names", function( assert ) {
"-dashy-hanger"
];

assert.expect( 16 );
assert.expect( 17 );

var curData,
div = document.createElement( "div" );
Expand Down
4 changes: 2 additions & 2 deletions test/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ test( "load() and unload() event methods", function( assert ) {
} );

QUnit.test( ".bind() and .unbind()", function( assert ) {
assert.expect( 3 );
assert.expect( 5 );

var $elem = jQuery( "<div />" ).appendTo( "#qunit-fixture" );

Expand All @@ -73,7 +73,7 @@ QUnit.test( ".bind() and .unbind()", function( assert ) {
} );

QUnit.test( ".delegate() and .undelegate()", function( assert ) {
assert.expect( 3 );
assert.expect( 5 );

var $div = jQuery( "<div />" ).appendTo( "#qunit-fixture" );

Expand Down
1 change: 1 addition & 0 deletions test/migrate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function expectWarning( name, expected, fn ) {
// Simple numeric equality assertion for warnings matching an explicit count
} else if ( expected && jQuery.migrateWarnings.length === expected ) {
equal( jQuery.migrateWarnings.length, expected, name + ": warned" );
equal( jQuery.migrateWarnings.asErrors.length, expected, name + ": warned as Errors" );

// Simple ok assertion when we saw at least one warning and weren't looking for an explict count
} else if ( !expected && jQuery.migrateWarnings.length ) {
Expand Down