596248 - Issues with current implementation of traits

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Issues I've encounter with current API and simplification proposal are described in details here:

https://gist.github.com/9d4cbaba79c57abb3873

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Proposede API changes

Associated pull request: http://github.com/Gozala/jetpack-sdk/pull/4

Please note: That patch doesn't removes current implementation neither it include changes to the current trait based modules, both are trivial tasks since changes are pretty small.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 475103 [details] [diff] [review]
Proposede API changes

Overall, these changes seem reasonable.

The only change I don't quite understand is the factoring out of "capabilities" into a separate trait with which objects must be composed if they want to hide their private properties.  Since all (or perhaps most) of the objects we compose from traits are going to want to hide private properties, it seems easier and less dangerous to build capabilities into traits than to require developers to manually mix them in.

Can you elaborate on the rationale for that change? The gist <https://gist.github.com/9d4cbaba79c57abb3873> just says the simplified API makes it possible, but it doesn't explain why it's better, and it seems like a change for the worse to me.

Also, don't you need to update Traits consumers when making these changes?

Otherwise, just two issues...

First, there is a test failure (test-capabilities.test: compose trait instance and create instance (exception)) on the latest Firefox nightly.

Second, the "capabilities" module is missing documentation (although it directs readers to <https://jetpack.mozillalabs.com/sdk/latest/docs/#module/jetpack-core/capabilities>).

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Sorry maybe I did not made myself clear I was not considering it being finished work I just wanted to know if the change seemed reasonable and if it is then it made sense to update all the docs, consumers etc...

I do see one test failure with latest nightly, but it was not happening when I've submitted this patch, probably something has to do with js engine changes and I will look into it. 

Regarding your point about capabilities, there are actually several reasons for that which I probably can summarize that it gives better separation of concerns and there for more granular control. It makes design explicit rather then implicit as it is now. In details:

1. One **problem** that I face quite often is that I have to do something like:

const MyTrait = Trait.compose({
 constructor: function MyTrait() this,
 .....
})
exports.MyTrait = function() MyTrait.apply(null, arguments)._public

As you may see my trait returns an instance with all it's privates since I need
to share those within the module scope. Then in order to safely object to public I have to employ wrapper function. This example is a simplified version of what happens in real life. But it illustrates pretty well the case where you need to work around current design with additional boilerplate.

2. Another problem with current design is that it is not obvious what happens behind the scene, in practice it's usually leads to the relaxation and reliance on some magic while there are always gotchas:
   
- If any "public" function returns |this| all the privates will get exposed
- If any API exports composed trait basically all the privates get's exposed as well since someone will be able to do YourTrait.compose({ get self() this }) and get all the privates using self property.
- If you share your trait with other module they both have to employ wrappers to expose API outside.

Probably even more which I don't have on top of my mind right now. I think it's better to keep design simple: just include capabilities in to the mix and return _public.

3. Proposed change makes possible and actually encourages Traits with zero capabilities, which makes them safe to share:

var WindowWrapperTrait = Trait.compose({
   _window: Trait.required,
   getElementById: ....
   createElement: ....
   .....
)}

This trait can do anything with the window but it does not has one, there for no capabilities at all. In a hands of consumer with a window it's a powerful tool:

function WindowWrapper() {
  return Trait.compose(WindowWrapperTrait, CapabilitiesTrait).create({
    _window: myWindow
  })._public
}

This example illustrates pretty well the case where both WindowWrapper and WindowWrapperTrait can be safely shared. With a current design it's not true as it was illustrated with an examples above. 

4. Finally I think that explicit design will make reviewing process far more simple

Comment 4

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #3)
> Sorry maybe I did not made myself clear I was not considering it being finished
> work I just wanted to know if the change seemed reasonable and if it is then it
> made sense to update all the docs, consumers etc...

Ah, sorry, I misunderstood!


> I do see one test failure with latest nightly, but it was not happening when
> I've submitted this patch, probably something has to do with js engine changes
> and I will look into it. 

Ok, cool.


> Regarding your point about capabilities, there are actually several reasons for
> that which I probably can summarize that it gives better separation of concerns
> and there for more granular control. It makes design explicit rather then
> implicit as it is now.

Hmm, I like separation of concerns and making things explicit.  On the other hand, if almost every consumer of Traits is a consumer of capabilities, then isn't it worth making capabilities implicit, especially if the goal is to reduce boilerplate?


In details:
> 
> 1. One **problem** that I face quite often is that I have to do something like:
> 
> const MyTrait = Trait.compose({
>  constructor: function MyTrait() this,
>  .....
> })
> exports.MyTrait = function() MyTrait.apply(null, arguments)._public
> 
> As you may see my trait returns an instance with all it's privates since I need
> to share those within the module scope. Then in order to safely object to
> public I have to employ wrapper function. This example is a simplified version
> of what happens in real life. But it illustrates pretty well the case where you
> need to work around current design with additional boilerplate.

Yup, and I like the alternative you are proposing, which is, as I understand it, to make this:

  const MyTrait = Trait.compose({ ... });
  function MyThing() {
    return MyTrait.create({ constructor: MyThing });
  }
  exports.MyThing = MyThing;

Among other advantages, this clearly differentiates between a Trait, which provides a set of public and private properties composed from one or more other traits (along with primitives for further composition), and a constructor, which provides an object factory for creating objects having certain traits.

Ideally, the two would be one and the same, but given the significant differences between them, which we can't abstract away very well, distinguishing between them seems useful.

Of course, if you wanted to reduce boilerplate even further, you would make Trait auto-generate such a constructor, making this:

  const MyTrait = Trait.compose({ ... });
  exports.MyThing = MyTrait.constructor;
Or even (provided you don't otherwise need to reference the trait):

  exports.MyThing = Trait.compose({ ... }).constructor;


> 2. Another problem with current design is that it is not obvious what happens
> behind the scene, in practice it's usually leads to the relaxation and reliance
> on some magic while there are always gotchas:
> 
> - If any "public" function returns |this| all the privates will get exposed
> - If any API exports composed trait basically all the privates get's exposed as
> well since someone will be able to do YourTrait.compose({ get self() this })
> and get all the privates using self property.
> - If you share your trait with other module they both have to employ wrappers
> to expose API outside.
> 
> Probably even more which I don't have on top of my mind right now. I think it's
> better to keep design simple: just include capabilities in to the mix and
> return _public.

If I understand this correctly, you are proposing that the example above become:

  const { PublicAPI } = require("capabilities");
  const MyTrait = Trait.compose({ ... }, PublicAPI);
  function MyThing() {
    return MyTrait.create({ constructor: MyThing })._public;
  }
  exports.MyThing = MyThing;

This does make the fact that you are hiding private properties more explicit.  However, it also adds boilerplate.  And if you forget "_public", then doesn't that give consumers access to private properties?


> 3. Proposed change makes possible and actually encourages Traits with zero
> capabilities, which makes them safe to share:
> 
> var WindowWrapperTrait = Trait.compose({
>    _window: Trait.required,
>    getElementById: ....
>    createElement: ....
>    .....
> )}
> 
> This trait can do anything with the window but it does not has one, there for
> no capabilities at all.

Perhaps I misunderstand, but isn't this just as possible with your other changes even if capabilities remained part of the default Traits implementation?

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

To summarize issue with merging capabilities and traits is that it makes a lot harder to use them without restrictions in the same module scope. Very often I just need to do something like this:

// trait for internal use
const MyPrivateTrait = Trait.compose({....});
// same trait as one above but it can be shared outside of module
const MyPublicTrait = Trait.compose(MyPrivateTrait, PublicAPI);
exports.Thing = function Thing() MyPublicTrait.create({ constructor: Thing })


If you absolutely want to reduce boilerplate then I would suggest another approach that will be cleaner and bit more flexible:  

const { PublicAPI } = require("capabilities");
const MyTrait = PublicAPI(
  Trait1, 
  Trait2,
  {
  // Anonymous trait descriptor
});
exports.MyThing = function MyThing() MyTrait.create({ constructor: MyThing })

You may just change API of capabilities which will allow you to them in the same way as
you would use raw traits and use traits.

Please also note that usually you'll do much more stuff in the constructor
and that's actually one of the most annoying issues with current implementation that uses auto generated constructors to reduce verbosity. 
 
All that being said I do think the best option at the moment is to keep this small boilerplate and maybe try to polish it later once we know what will work best for us.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

During my free time I made a two libs for node.js / browsers that implement my proposal. I have slightly adjusted API but I'm quite happy with a result. Few other people tried it and I got positive feedback from them as well. I would suggest reviewing those libs and getting them into jetpack as they are there.  

https://github.com/Gozala/membrane-traits
https://github.com/Gozala/light-traits  

What do you think Myk ?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #7)
> https://github.com/Gozala/membrane-traits
> https://github.com/Gozala/light-traits  

I think it's a good idea generally, although I haven't looked at the specific implementations.  Let's plan to evaluate and hopefully land this updated API and implementation in the next development cycle after the first beta release.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: https://github.com/Gozala/light-traits

This is a formal review request, actual code is on the links specified in the previous comments. Suggested API is in details described under: https://github.com/Gozala/light-traits/blob/master/docs/light-traits.md

This can be just cloned to the packages folder under jetpack-sdk to play or to see docs.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 497703 [details]
https://github.com/Gozala/light-traits

Ok, I've reviewed the traits code, the documentation describing the new API, and documentation for the concept of traits and the traits.js library on which this is based.

Overall, I'm fairly comfortable with this API, and I have satisfied my previous concerns about membrane integration (since even in the common case it's reasonable to make membranes a distinct trait that one explicitly applies to objects with private properties) and constructor auto-generation (as defining constructors manually isn't that much more code and clearly distinguishes between the different concepts of constructors and traits).

However, I do think trait composition should remain a separate operation from trait construction, as it is in traits.js and in your original implementation, i.e. with a Trait.compose() method instead of multiple arguments to the Traits constructor, since construction and composition are different operations, and it's beneficial to have callers be explicit about which operation they are performing.

Nevertheless, the mechanism for resolving conflicts in this API while composing traits (a resolve() method on Trait instances) is certainly preferable to the one in traits.js (a Traits.resolve() method that operates on the next argument), so the actual change is small: just expose the compose method to callers instead of routing composition through the constructor.


The documentation needs a lot of work.  It intersperses a bunch of information about property descriptor maps that won't be as useful to the majority of potential users of the API, and it has a variety of issues with code and prose formatting and clarity.  I've edited it to address these issues and put up the edited version as a gist:

https://gist.github.com/e3255aa2e956730b1695


The tests also don't work, but presumably that's because they are written for a different testing environment.


Otherwise, my review comments are mostly about improving the readability and conventionality of the code, so it fits in with other Mozilla code better, and other Mozilla hackers can more easily read, understand, and modify it.

In a programming-in-the-large project such as Jetpack, the number of whose contributors are expected to grow quite significantly over time, it's useful to stick to a set of code style conventions, even if they aren't exactly the style you would prefer, in order to make it easier for others to read and reason about your code.

Thus, throughout the code, make sure to use the following conventions:

* end statements with semi-colons (except inside one-statement one-line blocks);
* don't cuddle elses;
* separate items in multi-line lists with commas (and multi-line conditional
  expressions with logic operators) appended to the preceding lines rather than
  prepended to the following lines;
* put the static operand on the right hand side of equality operations;
* insert blank lines between major sections of code (including before comments);
* consistently place one-line conditional blocks on the next line;
* let is the new var (both in code and in documentation, since in this case our
  target audience for the documentation is API developers, who can be expected
  to know about and use `let`).


Code-specific comments follow.


> // shortcuts
> var _getOwnPropertyNames = Object.getOwnPropertyNames
> ,   _getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor
> ,   _defineProperty = Object.defineProperty
> ,   _getPrototypeOf = Object.getPrototypeOf
> ,   _keys = Object.keys
> ,   _create = Object.create
> ,   _freeze = Object.freeze
> ,   _prototype = Object.prototype
> ,   _hasOwn = Object.prototype.hasOwnProperty
> ,   _toString = Object.prototype.toString
> ,   _forEach = Array.prototype.forEach
> ,   _slice = Array.prototype.slice

These shortcuts make the code harder for someone unfamiliar with them to read (one has to keep referring back to them to remember what they represent). Shortcuts are worthwhile for particularly long names that are used frequently and whose long names would make the code that uses them less readable (f.e. by pushing a statement onto a second line).

Of these, however, only _hasOwn seems to fit that bill, since the others are not long enough to make the code less readable and/or not used frequently.  In fact, several aren't used at all.  So they should be inlined into the code instead of having shortcuts defined for them.

As for _hasOwn, since the code only ever does _hasOwn.call(), it would be better to define it as a lambda that wraps the call to call() as well and call it _hasOwnProperty to make it easier to understand while reading through the code without having to refer back to the alias declaration, i.e.:

function _hasOwnProperty(o, p) Object.prototype.hasOwnProperty.call(o, p);


> // constants
> ,   ERR_CONFLICT = 'Remaining conflicting property: '
> ,   ERR_REQUIRED = 'Missing required property: '

These should be declared `const`.


> function exclude(keys, trait) {
>   var exclusions = Map(keys)
>   ,   result = {}
>   ,   keys = _keys(trait)
>   keys.forEach(function(key) {

The `keys` declaration redeclares an argument, generating a warning.  The declaration should either use a different name, reassign keys without redeclaring the variable (i.e. assign it outside of the `var` declaration), or just call .forEach() on Object.keys(trait) directly.


> function rename(map, trait) {

The code in this function (and other such functions, although this one the most) is hard to read and reason about, since it is complex, compact, and not well documented.  It's possible that the code is as simple as it can be--fair enough--although if the logic can be simplified, then it's worth doing so, even at the cost of un-refactoring (i.e. repeating) some code.

But even if it isn't possible to simplify it, code like this should incorporate formatting (such as blank lines) and commentary that explains not only the behavior of the code but also the reasoning behind it. For example, based on my understanding of this function and the concept of traits, the function might look something like this:

function rename(map, trait) {
  let result = Object.create(Trait.prototype, {}),
      keys = Object.keys(trait);

  // Loop over all the properties in the trait and copy them to a new trait,
  // renaming them as appropriate.
  keys.forEach(function(key) {
    // If the property is in the map, and it isn't a required property
    // (which should never need to be aliased because required properties
    // never conflict), then we must try to rename it.
    if (_hasOwnProperty(map, key) && !trait[key].required) {
      let alias = map[key];

      // If the result trait already has the alias, and it isn't a required
      // property, that means the alias conflicts with an existing name for a
      // provided property, so we have to mark it as a conflicting property.
      // Otherwise, everything is fine, so we assign the value to the alias
      // in the result trait.
      if (_hasOwnProperty(result, alias) && !result[alias].required)
        result[alias] = Conflict(alias);
      else
        result[alias] = trait[key];

      // Regardless of whether or not the rename was successful, we check to see
      // if the original name exists in the result trait; if it doesn't, we mark
      // it required, to make sure the caller provides another value for the old
      // name, which methods in the trait might continue to reference.
      if (!_hasOwnProperty(result, key))
        result[key] = Required(key);
    }

    // Otherwise, either the property isn't in the map (thus the caller is not
    // trying to rename it) or it is a required property.  Either way, we don't
    // have to alias the property, we just have to copy it to the result trait.
    else {
      // The property isn't already in the result trait, so we copy it over.
      if (!_hasOwnProperty(result, key))
        result[key] = trait[key];

      // The property is already in the result trait.  If it's a provided
      // property, that means the caller renamed another property to the same
      // name as this property, which creates a conflict, so we have to mark
      // it as a conflicting property.
      else if (!trait[key].required)
        result[key] = Conflict(key);
    }
  })

  return result;
}


> /**
>  * Function generates custom properties descriptor of the `object`s own
>  * properties. All the inherited properties are going to be ignored.
>  * Properties with values matching `required` singleton will be marked as
>  * 'required' properties.
>  * @param {Object} object
>  *    Set of properties to generate trait from.
>  * @returns {Object}
>  *    Properties descriptor of all of the `object`'s own properties.
>  */
> function toTrait(properties) {

The comment here says the function generates and returns a "properties descriptor" that is of type Object, but it actually returns a trait that is more precisely described as being of type Trait (retVal instanceof Trait === true).  It should say (including fixes for some other minor grammatical and clarity issues):

/**
 * Generate a trait based on the properties of the given `object`.
 * Only the `object`'s own properties are acquired by the trait; inherited
 * properties are ignored.  Properties with values matching the `required`
 * singleton are marked as required properties.
 *
 * @param {Object} object
 *    the set of properties from which to generate the trait
 *
 * @returns {Trait}
 *    the generated trait
 */


>     trait = trait instanceof Trait ? trait : toTrait(Object.create({}, trait))

Why create a new object from the trait before passing it to toTrait? toTrait treats it as an object, and it will be coerced to one if it's a primitive value.  The only other caller (the Trait function) doesn't do this, and it seems unnecessary.


> var TraitProto = Trait.prototype = _create(Trait.prototype,

Why define TraitProto? It seems unnecessary, as you should be able to just use Trait.prototype for its one accessor.


> , create: { value: function create(proto) {
>     var properties = {}
>     ,   keys = _keys(this)
>     if (undefined === proto) proto = _prototype
>     if (proto) {
>       if ('' + proto.toString == '' + _toString) {
>         _defineProperty(proto, 'toString',  {
>           value: TraitProto.toString
>         })
>       }
>       if ('' + proto.constructor == '' + Object) {
>         _defineProperty(proto, 'constructor', {
>           value: Trait.prototype.constructor
>         })
>       }
>     }
>     keys.forEach(function(key) {
>       var descriptor = this[key]
>       if (descriptor.required) {
>         if (key in proto) {
>           return properties[key] = _getPropertyDescriptor(proto, key)
>         }
>         else throw new Error(ERR_REQUIRED + '`' + key + '`')
>       } else if (descriptor.conflict) {
>         throw new Error(ERR_CONFLICT + '`' + key + '`')
>       } else {
>         properties[key] = descriptor
>       }
>     }, this)

The anonymous function passed into keys.forEach doesn't always return a value, which generates a warning.  It should be consistent within the function (i.e. either always return, or never do).

And the inner if/else should be consistent in its usage of brackets.  In this case, since both the if and the else blocks are one-liners, it shouldn't use them, i.e.:

        if (key in proto)
          return properties[key] = _getPropertyDescriptor(proto, key);
        else
          throw new Error(ERR_REQUIRED + '`' + key + '`');


>   return Object.create
>   ( BaseAssert.apply(null, arguments)
>   , AssertDescriptor
>   )

When it's possible to fit such calls on a single line, it's better to do so, as it makes them easier to read, i.e.:

  return Object.create(BaseAssert.apply(null, arguments), AssertDescriptor);


> //      var g = TGreeting.create({ name: 'membrane', lastName: 'trait' })
> //      '_greeting' in g    // false <- privates are not exposed
> //      'lastName' in g     // false <- only trait properties are exposed

Hmm, maybe I misunderstand, but don't we want consumers to be able to create instances with additional properties besides the ones they acquire from their traits?

Comment 11

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Hi Myk,

I've tried to address all the issues you've raised, but I have few
objections on few points and I hope you'll find them reasonable:

- put the static operand on the right hand side of equality operations

There is good rational on putting static operand on left side which is also
advertised by Dug Crockford. The reason for such a good practice is a prevention
/ early catch of errors: For example coder may make a typo and type `foo = true`
instead of `foo == true` which may or may not be caught and that will lead to
unexpected behavior of program. While `true = foo` will guarantee an exception
as soon as the line is evaluated. Most likely review will catch such errors
but I think it is still better to promote good practice (specially for those
who just start hacking).

- `let` is the new `var`

I totally agree that `let` is superior to `var` but I really would love to keep
this library compatible with as many environments as possible and in many of
of those `let` is not supported. Also it should be noticed that in current
implementation I use `var` only at the top of the lexical scope in which case
it's behavior would be exactly the same as `let`'s.

I've integrated your doc changes with only one change:

    - let PointTrait = Trait.compose({ ... });
    + let PointTrait = Trait.compose(T1, T2, T3);

I think you're original code was bit misleading.

I still have to port tests (but would prefer a commonjs adapter that will map
test to harness format).

All the updated work is here:

https://github.com/Gozala/light-traits/tree/mozilla

I also would like to propose to splitting this task as follows:

1. Traits
2. Membranes

And finally how are we going to integrate this work as a package or just by
copying module into one of the existing packages ?.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #11)
> There is good rational on putting static operand on left side which is also
> advertised by Dug Crockford. The reason for such a good practice is a
> prevention
> / early catch of errors: For example coder may make a typo and type `foo =
> true`
> instead of `foo == true` which may or may not be caught and that will lead to
> unexpected behavior of program. While `true = foo` will guarantee an exception
> as soon as the line is evaluated. Most likely review will catch such errors
> but I think it is still better to promote good practice (specially for those
> who just start hacking).

I understand the point, but such errors are relatively rare in my experience and easily caught (not only by review but also from the JavaScript strict warnings they generate), and they are probably going to become even rarer with the advent of the strict equality operator (===).

And having the static operand on the right hand side makes such operations easier to read.  It is also by far the most conventional way of writing them in JavaScript (both inside and outside the Mozilla project), not to mention other programming languages, based on the code I've seen.

So its small cost in error detection is well worth the readability gains.


> - `let` is the new `var`
> 
> I totally agree that `let` is superior to `var` but I really would love to keep
> this library compatible with as many environments as possible and in many of
> of those `let` is not supported. Also it should be noticed that in current
> implementation I use `var` only at the top of the lexical scope in which case
> it's behavior would be exactly the same as `let`'s.

Sure, that's reasonable.  Just add a comment at the top that states that var is being used in the module in order to make it reusable in environments in which let is not yet supported.


> I've integrated your doc changes with only one change:
> 
>     - let PointTrait = Trait.compose({ ... });
>     + let PointTrait = Trait.compose(T1, T2, T3);
> 
> I think you're original code was bit misleading.

Yup, you're right.


> I still have to port tests (but would prefer a commonjs adapter that will map
> test to harness format).

Brian might be working on something like this; cc:ing him.


> I also would like to propose to splitting this task as follows:
> 
> 1. Traits
> 2. Membranes

We also need to update the consumers of the existing traits API to this new one.


> And finally how are we going to integrate this work as a package or just by
> copying module into one of the existing packages ?.

Existing package, I think, i.e. api-utils, where it can replace the existing traits.js implementation.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Myk can we make another review round ? Also if you won't mind that I'd like to integrate this package to an api-utils once you're happy with a code, since that would save me from maintaining two different repos.

So currently implementation of traits lives here:
https://github.com/Gozala/light-traits/tree/mozilla

And adapter for CommonJS tests is a pull request:
https://github.com/mozilla/addon-sdk/pull/93

Comment 14

[Myk Melez [:myk] [@mykmelez]]

I haven't reviewed the CommonJS test adapter yet, so I haven't tested the light traits code, but the code itself looks reasonable, except for some continuing inconsistency with the following code conventions:

* end statements with semi-colons (except inside one-statement one-line
blocks);
* separate items in multi-line lists with commas (and multi-line conditional
  expressions with logic operators) appended to the preceding lines rather than
  prepended to the following lines;
* put the static operand on the right hand side of equality operations;
* insert blank lines between major sections of code (including before
comments).


Also, there are a couple additional issues in assert.js:

>       else if (difference = findNonEquivalentPropertyName(actual, expected)) {

Enclose the assignment in an additional pair of parentheses so Gecko's classic strict checker doesn't warn about it, i.e.:

       else if ((difference = findNonEquivalentPropertyName(actual, expected))) {


>   equalTraits: {
>     value: function equivalentTraits(actual, expected, message) {
>       ...
>   }}

Indent closing braces for such blocks relative to their opening braces to make it easier to trace the closing braces back to their opening braces, i.e.:

  equalTraits: {
    value: function equivalentTraits(actual, expected, message) {
      ...
    }
  }

Also, this would need to be converted into a patch/pull request for the addon-sdk repository.  And we should just call this "traits.js", replacing the existing implementation (and converting existing consumers to use the new, simpler implementation), although I'm ok with doing that in multiple steps.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #13)
> And adapter for CommonJS tests is a pull request:
> https://github.com/mozilla/addon-sdk/pull/93

Coolness.  Comments in the pull request!

Comment 16

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to pull request

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 510930 [details]
Pointer to pull request

Getting close, but still some issues.  In particular, some of the comments are hard to grok or contain typos/grammar issues.  I've tried to clarify them based on my understanding of the implementation.  Comments in the pull request!

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Pointer to pull request

Comment 19

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 512759 [details]
Pointer to pull request

Addressed issues highlighted in the review. Also this time pull request aggregated added changes and they show up as in diff view below your comments and also added commits are added at the bottom of the discussion tab.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 512759 [details]
Pointer to pull request

Looks great, r=myk!