584707 - Object composition without compromising privacy.

Status: RESOLVED FIXED

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

Description

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

Attachment: protoype implementation

This is a prototype addressing few concerns highlighted under this thread on mailing list: http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/dbaec9800830bded#

Implementation has changed slightly since then and best place to look at the most uptodate version is:

http://github.com/Gozala/jetpack-sdk/compare/es5...capabilities

Implementation is based on top of:
https://bugzilla.mozilla.org/show_bug.cgi?id=580389

Comment 1

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

this is an updated link to track the latest version:
http://github.com/Gozala/jetpack-sdk/compare/es5@580389...capabilities@584707

Comment 2

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

Hey Irakli, what's the best way to download a patch of the latest version that I can apply to my local repo for evaluation?

Comment 3

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

I think first it's very good to look at the graph to see what is build on top of what: http://github.com/Gozala/jetpack-sdk/network

master is usually in sync with a tip from official repo.

I guess something like this would be the easiest option to apply changes to local hg repo

git clone [e-mail]:Gozala/jetpack-sdk.git jetpack-git  && cd jetpack-git
git diff origin/master origin/capabilities@584707 | patch -p 1 -d /path/to/hg/repo

of course you can use any other branch instead of capabilities@584707 to see more examples what it provides

If you don't mind messing with git probably it will be easier, just clone and pull the changes when you need the latest changes.

Comment 4

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

Comment on attachment 463158 [details] [diff] [review]
protoype implementation

Very cool work!  Absent new syntax in a future rev of the language, the quite conventional leading underscore seems like a fine way to identify private properties.

However, that made me wonder how custom iterators are handled, and it looks like they don't work.  With a test like the following:

    exports['test:custom iterator'] = function(test) {
      let { Base } = require('private/core');
      let Sub = Base.extend({
        foo: "foo",
        bar: "bar",
        baz: "baz",
        __iterator__: function() {
          yield 1;
          yield 2;
          yield 3;
        }
      });
      let (i = 0, sub = Sub()) {
        for (let item in sub)
          test.assertEqual(++i, item, "iterated item has the right value");
      };
    }

There are two possible results.  If the test is run before the others, I see:

  error: TEST FAILED: test-core.test:custom iterator (failure)
  error: fail: iterated item has the right value (1 != "foo")
  ...
  error: fail: iterated item has the right value (2 != "bar")
  ...
  error: fail: iterated item has the right value (3 != "baz")

If the test is run after the others, on the other hand, I see:

  ..............error: TEST FAILED: test-core.test:custom iterator (exception)
  error: An exception occurred.
  Traceback (most recent call last):
    File "resource://jetpack-core-jetpack-core-lib/timer.js", line 58, in notify
      this._callback.apply(null, []);
    File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 259, in 
      timer.setTimeout(function() { onDone(self); }, 0);
    File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 284, in runNextTest
      self.start({test: test, onDone: runNextTest});
    File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 302, in start
      this.test.testFunction(this);
    File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 57, in runTest
      test(runner);
    File "resource://jetpack-core-jetpack-core-tests/test-core.js", line 226, in 
      __iterator__: function() {
    File "resource://jetpack-core-jetpack-core-lib/private/core.js", line 160, in 
      let internal = _create(proto, properties);
    File "resource://jetpack-core-jetpack-core-lib/private/core.js", line 84, in 
      let object = Object.create({}, properties);
  TypeError: value is not a non-null object

Perhaps __iterator__ could be special-cased as a "public" property?


diff --git a/packages/jetpack-core/docs/private/core.md b/packages/jetpack-core/docs/private/core.md

+![Diagram](http://yuml.me/diagram/scruffy;dir:TB;/class/[prototype{bg:orange}]proto ->[outerState{bg:green}], [prototype]-[_privateMember], [prototype]-[publicMember], [prototype]-[_privateMethod], [prototype]-[publicMethod], [outerState]proto->[innerState{bg:red}], [outerState]<-.-[publicMember{bg:yellow}], [outerState]^-[publicMethod.bind（innerState）{bg:yellow}], [innerState]<-.-[publicMethod.bind（innerState）], [innerState]^-[_privateMethod], [innerState]^-[_privateMember], [innerState]<-.-[publicMember], [publicMethod.bind（innerState）]-.-[publicMethod] "diagram")

This diagram is very cool!  It's a bit hard to understand, though, and I wonder if it might be possible to make it more grokkable with a caption and/or legend describing what it depicts.  Also, it seems like the kind of thing we should take a snapshot of and include as an image with the SDK rather than making people click through to another website.

And this documentation could really use some prose descriptions of the purpose and usage of this module in addition to the example code!


\ No newline at end of file

Nit: newline at end of file!


diff --git a/packages/jetpack-core/lib/private/core.js b/packages/jetpack-core/lib/private/core.js

+function Create(proto) {
+  return canFreeze ? function create(options) {
+    let internal = _create(this.prototype, {
+          _guid: { value: ++GUID },
+        }),
...
+    let internal = _create(proto, {
+          _guid: { value: ++GUID },
+        }),

Nit: the object literals passed to _create have a trailing comma in their property lists, which causes Firefox to issue a strict warning about trailing commas not being legal in ECMA-262 object initializers.  To avoid that warning, it would be better if they didn't have the trailing commas.


+function Extend(proto) {
+  return canFreeze ? function extend(extension, properties) {
+    if (properties && extension)
+      properties = mix(properties, getOwnPropertiesDescriptor(extension));
+    else if (!properties)
+      properties = getOwnPropertiesDescriptor(extension);
+    let proto = _create(this.prototype, properties);
+    let Class = function (options) Class.create(options);

Nit: given that the intent is not to implement class-based inheritance, perhaps it would make more sense to call this Constructor or Type?


diff --git a/packages/jetpack-core/tests/test-core.js b/packages/jetpack-core/tests/test-core.js

+exports['test:instances may not be hackable'] = function(test) {
+  let SECRET = 'There is no secret!',
+      secret = null;
+  let { Base } = require('private/core');
+  let Class = Base.extend({
+    _secret: null,
+    protect: function(data) this._secret = data
+  });
...
+  let Class = Base.extend({

Class gets redeclared thrice in the same scope here, which I have been under the impression is a syntax error (so I'm not sure why it isn't causing this test to fail).  It should only be declared with |let| only once in a given scope.

Perhaps this test function would be better broken up into several test functions?  Alternately, you can create separate scopes within the function, either with block statements containing let definitions or with let statements (which combine a block statement with a let definition), i.e.:

function() {
  {
    let Class = Base.extend({...});
    ...
  }
}

function() {
  let (Class = Base.extend({...})) {
    ...
  }
}


Atul should take a look at this, too, as he was part of the original decision to use lexical closures to hide private properties.  Requesting review from him, but Atul: note that the code in this patch isn't the latest code to review.  For that, see the github repo.

Comment 5

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

Attachment: Addressing review comments and discussion notes

commit link: http://github.com/Gozala/jetpack-sdk/commit/db5b5ee238559c14cd7d4edb74f86608aab1f1df

Comment 6

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

To get the latest changes

git clone [e-mail]:Gozala/jetpack-sdk.git jetpack-git  && cd jetpack-git
git diff origin/master origin/traits | patch -p 1 -d
/path/to/hg/repo

or just clone and pull traits branch

Comment 7

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

Comment on attachment 468677 [details] [diff] [review]
Addressing review comments and discussion notes

Great!  Traits is more complicated than mixins, but it does elegantly solve a few problems with mixins, and it seems like it will be relatively easy to use once one picks up the basics.

I only found minor issues around the documentation, which are marked in GitHub review notes on the commit <http://github.com/Gozala/jetpack-sdk/commit/db5b5ee238559c14cd7d4edb74f86608aab1f1df>.

Regarding http://github.com/Gozala/jetpack-sdk/commit/fb099a7d7f429e11621c2d1ca60910c0d737b1bf, perhaps you can put together a patch (or a git branch) that contains the full set of all changes implementing traits with private property hiding?

Then I can do one final review.

Atul: you don't need to review this, but it would be useful to provide feedback; in particular, whether or not you know of a fatal flaw that would prevent us from using this approach to hide private properties.

Comment 8

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

Myk I've addressed all the review comments except one regarding 'TraitDescriptor' since it actually constructs descriptors for traits, I believe my doc comment was just misleading so I have updated it.

Apart from that:

- I changed few lines which where relying on function `caller` property which is as you mentioned in other review is illegal in ES5 strict mode.
- Added some more details regarding trait's constructor behavior.
- Removed properties that I had only for debugging purposes.

All the commits related to this bug are in the branch:

http://github.com/Gozala/jetpack-sdk/tree/capabilities@584707

Your last review included all the changes up to this commit:
http://github.com/Gozala/jetpack-sdk/commit/6ddc27574ec1fb8fb6f6860d9fc7add39f2e9776

All the new changes since last review can be viewed:
http://github.com/Gozala/jetpack-sdk/compare/6ddc27574ec1fb8fb6f6860d9fc7add39f2e9776...capabilities@584707

Comment 9

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

Attachment: Addressing review commets

Comment 10

[Atul Varma [:atul]]

Comment on attachment 470406 [details] [diff] [review]
Addressing review commets

This is cool! I like it... thanks also for the great documentation.

While I didn't fully review the code, I did notice a misspelled word in traits.js, "argumnet".

Comment 11

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

Attachment: Fixing spelling errors

commit link: http://github.com/Gozala/jetpack-sdk/commit/676d1ff03262dcb082cc1cfbdf8e68efadeb6941

Comment 12

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

Comment on attachment 471142 [details] [diff] [review]
Fixing spelling errors

Looks good, r=myk!

Note: this doesn't need another round of feedback from Atul, since he was providing feedback on the general approach, not the specific implementation, so cancelling that feedback request.

Comment 13

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

Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/c68dbc0d5844.

I'm looking forward to using this!

Comment 14

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

One question: is bug 585891 really a blocker for this?  If so, I'll have to back this out, as that bug isn't fixed yet.  Otherwise, we should remove that bug from the list of this bug's blockers (or file another bug about the specific part of this bug's functionality that is being impaired by that bug).

Comment 15

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

No it's not a blocker for this on since I have worked around the case where the  bug 585891 happens. But I've added bug 585891 to dependencies so that we may

Comment 16

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

(In reply to comment #15)
> No it's not a blocker for this on since I have worked around the case where the
>  bug 585891 happens. But I've added bug 585891 to dependencies so that we may

Sorry for this unfinished comment :) I started replying but then I changed dependencies and comment got submitted.

Anyway original implementation had a dependency on this feature, but it was worked around so it was not a blocker anyway, I just had it listed in the dependencies so that I could remove workaround once bug was fixed.

Current implementation does not depends on this functionality at all so I have removed bug from dependencies.

Comment 17

[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".