r84985 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r84984‎ | r84985 | r84986 >
Date:22:08, 29 March 2011
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Swtiched to using console.log if present so that when errors occur inside of modules they are shown, even if you aren't in debug mode. This helps avoid the common pitfall of forgetting to export a symbol to the global space, because in debug mode everything is exported to the global space whereas in production mode it's wrapped in a closure.
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -718,8 +718,12 @@
719719 }
720720 }
721721 } catch ( e ) {
722 - mw.log( _fn + 'Exception thrown by ' + module + ': ' + e.message );
723 - mw.log( e );
 722+ // This needs to NOT use mw.log because these errors are common in production mode
 723+ // and not in debug mode, such as when a symbol that should be global isn't exported
 724+ if ( console && typeof console.log === 'function' ) {
 725+ console.log( _fn + 'Exception thrown by ' + module + ': ' + e.message );
 726+ console.log( e );
 727+ }
724728 registry[module].state = 'error';
725729 // Run error callbacks of jobs affected by this condition
726730 for ( var j = 0; j < jobs.length; j++ ) {

Sign-offs

UserFlagDate
Krinkleinspected23:58, 30 March 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r85140Fixed issue where reference error can be thrown - response to comments on r84985tparscal18:05, 1 April 2011
r852561.17wmf1: MFT r80813, r80815, r83798, r84459, r84729, r84820, r84921, r84985,...catrope14:13, 3 April 2011
r85435MFT: r84431, r84464, r84543, r84553, r84573, r84574, r84577, r84729, r84765, ...demon14:00, 5 April 2011
r856141.17wmf1: Fix botched merge of r84985 in r85256, was causing JS errorscatrope11:06, 7 April 2011
r856151.17: Undo botched merge of r84985 in 85435, was causing JS errorscatrope11:08, 7 April 2011

Comments

#Comment by Krinkle (talk | contribs)   10:33, 1 April 2011

Please use window.console instead of console. For one because right now it throws a ReferenceError exception when console is undefined! (Marking FIXME for that)

...whereas checking an object member of window never fails with a ReferenceError, it just returns null / undefiend -> implied false. Another solution would be to check typeof == 'undefined', but using window is shorter and also fixes the high unlikely case would there be a local variable called "console".

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:05, 1 April 2011

Excellent catch - I followed up in r85140

#Comment by Krinkle (talk | contribs)   10:14, 7 April 2011

This revision should not have been merged, it's destroying javascript execution on all wikimedia wikis in debug mode and/or if the catch() is triggered.

Reason being that _fn (which you are referring to) were introduced in r83658 (as _method) and renamed to _fn in r84276

#Comment by Catrope (talk | contribs)   11:10, 7 April 2011

Unmerged from 1.17wmf1 in r85614 and from REL1_17 in r85615.

#Comment by Krinkle (talk | contribs)   10:18, 7 April 2011

http://en.wikipedia.org/wiki/Main_Page?debug=true (enable the resourceloader Navigtion popups gadget, which triggers the catch() on en.wikipedia ) 


* _fn is not defined
* console.log( _fn + 'Exception thrown by ' + module + ': ' + e.message );
* mw.util is undefined
[break] importScriptURI('[http://meta.wikimedia...s&action=raw&ctype=text/javascript'); http://meta.wikimedia...s&action=raw&ctype=text/javascript');]
importScriptURI is not defined
[break] importScriptURI('[http://meta.wikimedia...s&action=raw&ctype=text/javascript'); http://meta.wikimedia...s&action=raw&ctype=text/javascript');]
syntax error
}
#Comment by Trevor Parscal (WMF) (talk | contribs)   16:58, 7 April 2011

So, the issue can be resolved by not using _fn?

#Comment by Catrope (talk | contribs)   16:59, 7 April 2011

It only affects 1.17wmf1 and REL1_17, and I already fixed it in those branches, see my comment.

#Comment by Krinkle (talk | contribs)   17:59, 7 April 2011

Yeah, using _fn is fine in trunk / 1.18 - no further action needed.

#Comment by Tim Starling (talk | contribs)   05:40, 6 May 2011

Can we have this feature back in the 1.17 branch please? It would make it easier to support beta users reporting RL-related bugs.

#Comment by Catrope (talk | contribs)   09:43, 6 May 2011

Alright, I'll merge this into 1.17. It was merged before but backed out because it caused JS errors.

Status & tagging log