View source for Labs/Jetpack/So You're Implementing a JEP

So you're implementing a JEP.

Most of this applies to the high-level APIs only, the ones covered by JEPs.  Low-level modules, which have mostly been written by Atul, follow different conventions.

= Landing your Implementation =

The process from JEP to landing has worked something like this so far:

* JEP first draft on wikimo.
* Ask Myk for review.
* He'll start a conversation in the [http://groups.google.com/group/mozilla-labs-jetpack user group] so everybody can offer feedback.
* Iterate on the JEP, updating the wiki, based on the conversation.
* Final JEP draft agreed upon.
* Ask Myk to review your patch.  He'll check that it more or less implements the JEP.
* Ask Atul to review your patch.  He'll check that it uses the Jetpack platform OK.
* Ask a domain expert to review your patch.

= Modules =

The module you're writing is a [http://wiki.commonjs.org/wiki/Modules/1.0 CommonJS module] and is similar to a Mozilla JavaScript module (<tt>.jsm</tt>).

All the properties you attach to the <tt>exports</tt> object are visible to the outside world.  Everything else is private.

=== Loading ===

Instances of your module are created by the [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/lib/securable-module.js#193 Cuddlefish loader], which is part of the Jetpack platform.

A new instance is created the first time an extension <tt>require()</tt>s it, but thereafter that instance is generally cached for that extension.  However, the loader is capable of creating multiple concurrent instances of your module should the need arise, so you shouldn't write your module such that it depends on there being only one instance of it at a time.  For each instance, the loader creates a <tt>Cu.Sandbox</tt> and evaluates your module's code inside it.

=== Unloading ===

Your module can be unloaded at any time, so it should be prepared, and it should clean up after itself.  (Restartless extensions, yeah?)  When exactly your module is unloaded is up to the Cuddlefish loader.

Cleaning up on unload is easy.  The [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/lib/unload.js <tt>unload</tt> module] provides two helper functions: <tt>when()</tt> and <tt>ensure()</tt>.  <tt>when()</tt> takes a callback, and it's called when your module is unloaded.  The callback should perform any necessary cleanup.  <tt>ensure()</tt> takes an object that defines an <tt>unload()</tt> method.  When you call <tt>require("unload").ensure(obj)</tt>, <tt>obj.unload()</tt> is called once and only once when your module is unloaded.

Note that all (proper) modules already clean up after themselves, and if the only cleanup your module needs to do is related to the resources of other modules, you might not even need to do anything at all.

If however your module creates its own resources, it should clean them up when it's unloaded.  Your unit test should ensure that it does.  See the [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/tests/test-xhr.js#32 XHR test] for an example of manually loading and unloading a module.

(Technically speaking <tt>when()</tt> and <tt>obj.unload()</tt> are called when the <tt>unload</tt> module is unloaded, not when your module is unloaded.  But since your module must first obtain an instance of the <tt>unload</tt> module to use <tt>when()</tt> and <tt>ensure()</tt>, your module's unload causes the <tt>unload</tt> instance to be unloaded.)

= Private Parts =

You make "private" members in Mozilla JavaScript code by prefixing their names with an underscore (e.g., <tt>this._dontTouchMe</tt>), but that's no good for Jetpack, where we're trying to enforce, you know, a security model.

Whenever you define a property on an object, stop and think if that object is exported -- if it's public.  Yes?  Is that property "private"?  Yes?  Then it's not really private at all.

Also, clients can apply your public methods to any arbitrary object by using <tt>call()</tt> and <tt>apply()</tt>.  You therefore can't be sure that <tt>this</tt> really refers to your object.  It might be an object that does something untoward when applied to the code you've written for yours.

For these reasons, define all public methods on <tt>this</tt> inside your class's constructor, and instead of using <tt>this</tt> inside those methods, use variables in the scope of the constructor.

<pre class="brush:js;toolbar:false;">
// BAD
function MyExportedObject(someProperty) {
  this._someProperty = someProperty;
};
MyExportedObject.prototype = {
  publicMethod: function () {
    return this.alsoPublic();
  },
  alsoPublic: function () {
    return this._privateHelper();
  },
  _privateHelper: function () {
    return this._someProperty;
  }
};

// GOOD
function MyExportedObject(someProperty) {
  const self = this;

  this.publicMethod = function () {
    return self.alsoPublic();
  };

  this.alsoPublic = function () {
    return privateHelper();
  };

  function privateHelper() {
    return someProperty;
  }
};
</pre>

= Client Callbacks =

=== try-catch-log ===

Always try-catch callbacks passed into your API and log the error with [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/lib/plain-text-console.js#88 <tt>console.exception()</tt>].   Two reasons:

# You don't want user exceptions interrupting control of your own code.
# Jetpack automatically logs all errors originating in the main event loop, but it misses others, like those in event listeners.

For the second reason, you should also try-catch-log your own code that's not called on the main event loop.

There is an [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/lib/errors.js <tt>errors</tt>] module that currently provides some related helpers and could stand to be expanded.  <tt>catchAndLog()</tt> returns a function that wraps a callback in a try-catch-log.

<pre class="brush:js;toolbar:false;">
// BAD
someObj.callback();

// GOOD
try {
  someObj.callback();
}
catch (err) {
  console.exception(err);
}
</pre>

=== <tt>this</tt> ===

Think about what object <tt>this</tt> should refer to inside your client's callback.  (It might be obvious, but if not, it should be specified in the JEP.)  In particular, be careful about calling a callback that isn't attached to an object.  If you call it as you normally would a function, <tt>this</tt> is the current global context.  What's the global context inside your module code?  It's not your module, by which I mean the <tt>exports</tt> object.   It's the <tt>Cu.Sandbox</tt> that Cuddlefish used to evaluate your code!  Not something that your clients should be able to access.

<pre class="brush:js;toolbar:false;">
// BAD
callback();

// BETTER
callback.call(exports);

// BEST
someRelevantObject.callback();
callback.call(someRelevantObject);
</pre>

= Exceptions =

Never throw raw strings, because <tt>console.exception()</tt>, which is ultimately used to log any exceptions in Jetpack, can't introspect a string to get at any additional information about where the error came from. Instead, throw a <tt>new Error()</tt>, which automatically adds metadata about the state of the stack at the time the error was thrown. This allows us to provide helpful tracebacks to developers.

Being friendly to developers is an important part of Jetpack.  Therefore, all error messages that developers might see should be informative and punctuated full sentences.

XPCOM exceptions generally aren't friendly, so they should be caught and friendly errors thrown in their place -- within reason of course.  You don't need to catch every possible XPCOM exception that might be thrown by a line.

<pre class="brush:js;toolbar:false;">
// BAD
let stream = Cc['@mozilla.org/network/file-output-stream;1'].
             createInstance(Ci.nsIFileOutputStream);
stream.init(file, openFlags, permFlags, 0);

// GOOD
let stream = Cc['@mozilla.org/network/file-output-stream;1'].
             createInstance(Ci.nsIFileOutputStream);
try {
  stream.init(file, openFlags, permFlags, 0);
}
catch (err if err.result === Cr.NS_ERROR_FILE_NOT_FOUND) {
  throw new Error("The file " + fileName + " does not exist.");
}
</pre>

= Testing =

Your module should come with unit tests.

If you use the <tt>unload</tt> module to clean up resources you've created, you should test that those resources are actually cleaned up when your module is unloaded.  See the [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/tests/test-xhr.js#32 XHR test] for an example of manually loading and unloading a module.

= Modularity =

Might parts of your code be useful to others?  Break them out into their own bugs so others can benefit.

= See Also =

* Atul wrote an older [https://wiki.mozilla.org/index.php?title=Labs/Jetpack/Reboot/Best_Practices&oldid=201780 best practices doc], but it's still totally relevant.
* Myk also wrote down some [https://wiki.mozilla.org/Labs/Jetpack/Design_Guidelines#Recommendations recommendations] that you should follow.
* Jetpack MXR: http://mxr.mozilla.org/labs-central/source/jetpack-sdk/
* [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/lib/ <tt>jetpack-core</tt> modules]
* Run <tt>cfx docs</tt> to see documented modules.
* Need to learn how to use an undocumented module?  Study how its [http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/tests/ unit test] uses it.