2509
Comment:
|
6573
|
Deletions are marked like this. | Additions are marked like this. |
Line 5: | Line 5: |
These are items that any [[ReviewerSchedule|code reviewer]] should keep in mind when reviewing JavaScript. |
|
Line 7: | Line 9: |
* '''JAVASCRIPT FILES MUST BE LINT-FREE''' * '''NO NEW SCRIPT FILES SHOULD BE PULLED IN BY THE YAHOO COMBO-LOADER''' |
* '''ALL JAVASCRIPT FILES MUST BE LINT-FREE''' * Note though that you can sometimes confuse XmlLint [1] * '''NO NEW SCRIPT FILES SHOULD BE PULLED IN BY THE YAHOO COMBO-LOADER''' (use [[JavaScriptBuildSystem|these instructions]] to add the scripts properly) * '''ANY DISPLAY OF TEXT INPUT MUST BE TESTED FOR PROPER ESCAPING.''' If this isn't tested, we don't know that we're safe against HTML injections. |
Line 14: | Line 18: |
* If 3 or more lines of strings are to be joined together, use the [].join("") form. | * If 3 or more lines of strings are to be joined together, use the `[str1, str2, ...].join("")` form. |
Line 17: | Line 21: |
* All functions, methods, and object properties should be described with JSDoc comment strings. | * All public or protected functions, methods, and object properties should be described with [[http://developer.yahoo.com/yui/yuidoc|JSDoc comment strings]] and [[http://developer.yahoo.com/yui/yuidoc#tags|tags]]. * Module-level private variables don't need full JSDoc tags. |
Line 20: | Line 25: |
* Function, method, and variable names should follow the [[http://www.python.org/dev/peps/pep-0008/|PEP 0008]] style guide (and no, this is not how the rest of the world does it). * Any string literal used more than once in a file should be assigned to a CONSTANT (a variable, really) so that the minifier can compile it to a short name. * functions should always return `null` not `undefined`. * To make conditionals explicit but readable, use the Y.Lang.isXXX methods: Y.Lang.isArray, Y.Lang.isObject, Y.Lang.isValue, and others. == XHTML, CSS, and Styling == |
|
Line 21: | Line 34: |
* CSS `url()` directives should have the file name surrounded by single-quotes: `url('spinner.png')` * XHTML attributes should be surrounded by double-quotes: `<input type="text"/>` * Setting a node's style explicitly with `setStyle()` should be avoided. * To set a node's style, it is preferable to add a CSS class to the node, and let the stylesheet handle the presentation. [2] * Please use the [[http://validator.w3.org|W3C markup validation service]] to check what you write, and to check what the DOM looks like after AJAX inserts new elements. |
|
Line 25: | Line 43: |
* `event.halt()` should be preferred over `event.stopPropagation()`. | * "`e`" should be used as the event object variable name in event handlers: `function onclick(e) {...}` * `event.halt()` should be preferred over `event.stopPropagation()`, `event.preventDefault()`, or `e.returnValue = false` |
Line 27: | Line 46: |
* Key event handling ''sucks''. The best cross-browser reference we've found to date is at http://unixpapa.com/js/key.html and for an example in our code, see `lib/lp/registry/templates/product-new.pt`. | |
Line 28: | Line 48: |
== JavaScript Modules == |
=== Javascript Module formatting === |
Line 32: | Line 51: |
* Module names should have the form `PROJECTNAME.foo`. | |
Line 36: | Line 54: |
* Script tags included for local development should be wrapped in <tal:devmode condition="devmode"></tal:devmode> tags so that un-minified scripts are only loaded for dev usage. | |
Line 37: | Line 56: |
== Namespaces == * Modules should declare a `PROJECTNAME` namespace. * A module's public functions should be attached to the `PROJECTNAME` namespace object. * A module's public functions should ''not'' be attached directly to the `Y` object. |
=== Naming Javascript Modules & Namespaces === * The file path, the module name, and the namespace should correspond to each other. * For example: * File path: canonical/launchpad/javascript/'''lp/calendar'''.js * Module name: Y.add(''' "lp.calendar" ''', ... * Namespace: var module = Y.namespace(''' "lp.calendar" '''); * A module's public functions should be attached to the namespace variable and '''not''' be attached directly to the `Y.namespace_name` variable. * Do this: {{{ var module = Y.namespace("lp.calendar"); module.my_public_function = function() {}; var my_private_function = function() {}; }}} * Don't do this, since Y.namespace() will create a new Y.lp object and Y.lp.calendar object if they don't already exist. {{{ Y.lp.calendar = Y.namespace("lp.calendar"); }}} * Don't do this, since it will be easier to change the module name and namespace, if you only have to do it in one place in the module. {{{ var module = Y.namespace("lp.calendar"); Y.lp.calendar.my_public_function = function() {}; // Should use `module` variable here. }}} * It will then be possible to use module functions like this: {{{ Y.use("lp.calendar", function() { Y.lp.calendar.my_public_function(); }); }}} |
Line 48: | Line 84: |
* All protected methods should have the `@protected` JSDoc tag, even ones like `initializer` and `renderUI`. | * All protected methods should have the `@protected` JSDoc tag, even ones like `initializer()` and `renderUI()`. |
Line 50: | Line 86: |
* Custom events should be documented immediately preceding the line on which they are published. | * Custom events should be documented on the line immediately preceding the line where they are published. |
Line 53: | Line 89: |
* Widget should have a functional example page. | * All Widgets should have a functional example page. |
Line 55: | Line 91: |
* The widget unit test suite should leave a clean DOM tree in between tests. | == Tests == * The unit test suite should leave a clean DOM tree in between each test case. == Footnotes == [1] ''It looks like literal regexps confuse it'', but OTOH, we recommend against using them. <<BR>>'''Wrong''', XmlLint rightly complains there is an unescaped less-than (<) in a document. Use `!= 0` or `!= 1` if you must embed script, but the right thing to do is to extract the script to a separate file. {{{ == XmlLint notices == lib/lp/registry/templates/product-new.pt 110: parser error : StartTag: invalid element name if (steps.search(/hidesearch/) < 0) { ^ }}} [2] For example, if you want to toggle an element's visibility using POJS, use our `unseen` class: {{{ // Make this element invisible. element1.addClass('unseen'); // Make this element visible again. element2.removeClass('unseen'); }}} |
JavaScript Review Notes
These are items that any code reviewer should keep in mind when reviewing JavaScript.
CRITICAL
ALL JAVASCRIPT FILES MUST BE LINT-FREE
Note though that you can sometimes confuse XmlLint [1]
NO NEW SCRIPT FILES SHOULD BE PULLED IN BY THE YAHOO COMBO-LOADER (use these instructions to add the scripts properly)
ANY DISPLAY OF TEXT INPUT MUST BE TESTED FOR PROPER ESCAPING. If this isn't tested, we don't know that we're safe against HTML injections.
Plain Old JavaScript
JavaScript blocks should be indented by 4 spaces.
If 3 or more lines of strings are to be joined together, use the [str1, str2, ...].join("") form.
- 1 and 2 character variable names are to be avoided.
- Regular expression literals should be avoided.
All public or protected functions, methods, and object properties should be described with JSDoc comment strings and tags.
- Module-level private variables don't need full JSDoc tags.
C++ style comments ("//") should only be used inside function bodies.
Per-line var statements are preferred inside function bodies.
Function, method, and variable names should follow the PEP 0008 style guide (and no, this is not how the rest of the world does it).
- Any string literal used more than once in a file should be assigned to a CONSTANT (a variable, really) so that the minifier can compile it to a short name.
functions should always return null not undefined.
- To make conditionals explicit but readable, use the Y.Lang.isXXX methods: Y.Lang.isArray, Y.Lang.isObject, Y.Lang.isValue, and others.
XHTML, CSS, and Styling
CSS files should come before JavaScript files in the document head.
CSS url() directives should have the file name surrounded by single-quotes: url('spinner.png')
XHTML attributes should be surrounded by double-quotes: <input type="text"/>
Setting a node's style explicitly with setStyle() should be avoided.
- To set a node's style, it is preferable to add a CSS class to the node, and let the stylesheet handle the presentation. [2]
Please use the W3C markup validation service to check what you write, and to check what the DOM looks like after AJAX inserts new elements.
Event Handling
"e" should be used as the event object variable name in event handlers: function onclick(e) {...}
event.halt() should be preferred over event.stopPropagation(), event.preventDefault(), or e.returnValue = false
Document-wide keypress handlers are dangerous, and should be detached as soon as they are no longer needed.
Key event handling sucks. The best cross-browser reference we've found to date is at http://unixpapa.com/js/key.html and for an example in our code, see lib/lp/registry/templates/product-new.pt.
Javascript Module formatting
Modules should have a JSDoc string with the @module or @submodule tag.
- Module-body content does not have to be indented.
- Module-wide constants should go at the top of the file, after the module JSDoc string.
Multi-line var statements may be used in the module-wide constants block.
Script tags included for local development should be wrapped in <tal:devmode condition="devmode"></tal:devmode> tags so that un-minified scripts are only loaded for dev usage.
Naming Javascript Modules & Namespaces
- The file path, the module name, and the namespace should correspond to each other.
- For example:
File path: canonical/launchpad/javascript/lp/calendar.js
Module name: Y.add( "lp.calendar" , ...
Namespace: var module = Y.namespace( "lp.calendar" );
- For example:
A module's public functions should be attached to the namespace variable and not be attached directly to the Y.namespace_name variable.
Do this:
var module = Y.namespace("lp.calendar"); module.my_public_function = function() {}; var my_private_function = function() {};
Don't do this, since Y.namespace() will create a new Y.lp object and Y.lp.calendar object if they don't already exist.
Y.lp.calendar = Y.namespace("lp.calendar");
Don't do this, since it will be easier to change the module name and namespace, if you only have to do it in one place in the module.
var module = Y.namespace("lp.calendar"); Y.lp.calendar.my_public_function = function() {}; // Should use `module` variable here.
It will then be possible to use module functions like this:
Y.use("lp.calendar", function() { Y.lp.calendar.my_public_function(); });
Widgets and Plugins
- Protected and private method names should be prefixed with an underscore.
All protected methods should have the @protected JSDoc tag, even ones like initializer() and renderUI().
Event handler method names should be prefixed with _on or _after.
- Custom events should be documented on the line immediately preceding the line where they are published.
All JSDoc strings for attributes should have @attribute, @type, and @default tags.
Objects such as Node instances should not be instantiated in the widget's ATTRS hash using value:, use valueFn: instead.
- All Widgets should have a functional example page.
(Not implemented) Widget example pages should follow the Example page Template.
Tests
- The unit test suite should leave a clean DOM tree in between each test case.
Footnotes
[1] It looks like literal regexps confuse it, but OTOH, we recommend against using them.
Wrong, XmlLint rightly complains there is an unescaped less-than (<) in a document. Use != 0 or != 1 if you must embed script, but the right thing to do is to extract the script to a separate file.
== XmlLint notices == lib/lp/registry/templates/product-new.pt 110: parser error : StartTag: invalid element name if (steps.search(/hidesearch/) < 0) { ^
[2] For example, if you want to toggle an element's visibility using POJS, use our unseen class:
// Make this element invisible. element1.addClass('unseen'); // Make this element visible again. element2.removeClass('unseen');