MediaWiki talk:Gadget-RenameWizard.js

Clean up

This script was pretty messy and full of syntax errors and mistakes. A few highlights:

• Fix race condition where the script fails because elements are not found.
  $( document ).ready was invoked with the return value of runRenameWizard instead of the function itself.
• Remove unused variables (a few variables where being set with values calculated from functions, only to be never used)
• Fix syntax errors (trailing commas in object literals, crashes in IE). They are tolerated in some browsers but they are illegal according to the spec and IE fails on it as expected.
• Avoid passing variables to $() (like $( "#"+divid );) it can fail when least expected. Not to mention potential security vulnerabilities when passing untrusted values through $() that can cause XSS attacks.
• Wrap the whole module in a closure to avoid leaking dozens of variables in the global scope that can easily conflict with other gadgets and plugins (an old patrol script I used crashed because of this, which lead to this clean up).
  • A few parts of the script actually abuse the fact that *everything* is global (href="javascript:postRenameRequest(..)"). Ever heard of event handlers? (on('click', postRenameRequest); // Passing the local function by reference)
• Missing semi-colons.
• Use undefined instead of null. The code was working by accident because when loosely compared (== instead of ===) null and undefined are "equal". When accessing a property that doesn't exist, the value is undefined not null.
• Consistently use single quotes (or double quotes). There are no "magic quotes" in javascript ("\n" === '\n').
• Consistently use tabs for indention (or spaces).
• Don't constantly re-query the entire document to select an element that is already queried before, over and over and over again:
  $('#foo').one(); $('#foo').two(); $('#foo').three();
  instead either cache the object and keep a reference:
  var x = $('#foo'); x.one(); x.two(); x.three();
  or use "chaining" (easy if you only need consecutive invokations):
  $('#foo').one().two().three();
  
• Consistent use of $('<tag>') vs. $(document.createElement('tag')).
• No need to use bracket notation for things like ['wiki'], it is longer and slightly slower. It is better just written in dot notation. At least be consistent!
• Learn the difference between properties and attributes. Don't use .attr('value') to read the value. Attributes don't update with live values from interactive use (e.g. if you type something in the input field, the attribute will remain empty!), they are only used to preset the default value from the html. To read the live value read from the property (.prop('value')). It happens to work when reading it through jQuery because it covers this common mistake for legacy reasons. Same goes for 'selected', 'checked' etc.
• Remove code that was commented out with no explaination.
• Add // <nowiki> to the top because ~~~~ was being expanded on-save (edit).
• Fix broken "Create Account Now" button. It was linking to a relative "index.php" which doesn't work as from /wiki/Page_name, accessing href="index.php" will go to /wiki/Index.php which is a 404 (Index.php). Should use wgScriptPath instead.
  • Also, run the name through encodeURIComponent when pasting it in a url.
• Declare dependencies, don't assume any modules will be present (user.tokens, mediawiki.user, mediawiki.util).
• Use plain objects ("singletons") instead of half-hacky using a class and instantiating it once and referring to the instance's name within the methods of the class:
  wvRenameWizard.prototype.fillRequest = function () { gRW.requestFromName = ..
  Either go with the model of using a class and properly refer to the current instance ("this"), or create a plain object ("singleton") and use the object's name. Not mixed up like this.

These aren't just mistakes made out of "quickly hacking it together" but many JavaScript and DOM fundamentals, so I hope the author(s) will learn from this :). In summary:

• Syntax (trailing commas)
• Bind handlers (not implied global variables and using global eval()-strings in <a href="javascript:.."
• Cache document queries instead re-quering the document over and over again.
• Difference between HTML attributes and properties of elements in the DOM.
• Difference between undefined and null, and why the old code worked by coincidence (two errors cancelling each other out)
• ResourceLoader dependencies
• Plain objects ("singletons") for groups of related functions and properties that don't need instantiating.

-- Krinkle (talk) 01:16, 13 January 2013 (UTC)

Mark historical?

@Wrh2: Since nothing is being linked to this gadget, should it just be marked as historical to be kept as reference or deleted outright? -- torty3 (talk) 12:19, 24 September 2015 (UTC)

I wasn't actually sure what this gadget was for, so thanks for clarifying in your edit that removed it from the list of user gadgets. I'd say mark it as historical, although if there is a desire to delete it then I wouldn't be opposed. -- Ryan • (talk) • 14:26, 24 September 2015 (UTC)

Yeah, I vaguely remember looking it up when it was still functional. I guess I'd rather keep it, though my point was more that no one would know it exists and it would be pretty hard to stumble across it in any way. -- torty3 (talk) 06:06, 25 September 2015 (UTC)