898394 - Implement an Object Emitter

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

The idea is to have a JSON like object that would emit a "set" event every time one of its property is modified, like a data store. This is useful for the App Manager, where we will be holding a JSON like object that will contain data about all the connected devices. We want any service to be able to use this object and be notified in case of changes.

Comment 1

[David Bruant]

DUP of bug 800355 ? (Object.observe will be the standard way to do what you want)
Meanwhile, you use Object.watch [1] or proxies [2] I think.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

Comment 2

[Victor Porof [:vporof][:vp]]

(In reply to David Bruant from comment #1)

Strongly agree with using Object.observe or Object.watch.

Comment 3

[Paul Rouget [:paul]]

Attachment: Patch v0.1

Proxy. Based on Benvie's code. Test are coming (I hit a clobber in the middle of my work).

Comment 4

[David Bruant]

Nit:

  function isObject(value) {
    let type = typeof value;
    return type == "object" ? value !== null : type == "function";
  }

can be replaced by:

  function isObject(value) {
    return Object(value) === value;
  }

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v0.2

with basic tests.

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 781687 [details] [diff] [review]
patch v0.2

Review of attachment 781687 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the comments below addressed.

::: browser/devtools/shared/event-emitter.js
@@ +190,5 @@
> +    return [this._wrappers.get(value), this._paths.get(value)];
> +  },
> +  get: function(target, key) {
> +    let value = target[key];
> +    let [wrapped, path] = this.wrap(target, key, value);

Isn't this.wrap going to throw if you'll try to get a value that hasn't been set yet? Please add tests for "get" as well, handling both primitives and objects.

@@ +199,5 @@
> +    let [wrapped, path] = this.unwrap(target, key, value);
> +    target[key] = value;
> +    this._emitter.emit("set", path, value);
> +  },
> +  getOwnPropertyDescriptor: function(target, key) {

Please test this as well.

@@ +217,5 @@
> +      }
> +    }
> +    return desc;
> +  },
> +  defineProperty: function(target, key, desc) {

Ditto.

@@ +231,5 @@
> +        [desc.set] = this.unwrap(target, "set "+key, desc.set);
> +      }
> +      Object.defineProperty(target, desc);
> +    }
> +  }

You should also handle |enumerate| and |iterate| if you want to avoid getting wrapped values in those cases. Do you want to? What do you think?

Comment 7

[Paul Rouget [:paul]]

Thanks for the review. I'll address these comments soon.


> You should also handle |enumerate| and |iterate| if you want to avoid getting
> wrapped values in those cases. Do you want to? What do you think?

I might add that.

Comment 8

[Brandon Benvie [:benvie]]

(In reply to Victor Porof [:vp] from comment #2)
> (In reply to David Bruant from comment #1)
> 
> Strongly agree with using Object.observe or Object.watch.

Object.observe is of course much preferable to this, but implementation is probably far off since no one has even begun work on it and it seems like it'd take a lot of work to implement.

Comment 9

[Victor Porof [:vporof][:vp]]

(In reply to Brandon Benvie [:bbenvie] from comment #8)
> (In reply to Victor Porof [:vp] from comment #2)
> > (In reply to David Bruant from comment #1)
> > 
> > Strongly agree with using Object.observe or Object.watch.
> 
> Object.observe is of course much preferable to this, but implementation is
> probably far off since no one has even begun work on it and it seems like
> it'd take a lot of work to implement.

Agreed.

Comment 10

[David Bruant]

(In reply to Brandon Benvie [:bbenvie] from comment #8)
> (In reply to Victor Porof [:vp] from comment #2)
> > (In reply to David Bruant from comment #1)
> > 
> > Strongly agree with using Object.observe or Object.watch.
> 
> Object.observe is of course much preferable to this, but implementation is
> probably far off since no one has even begun work on it and it seems like
> it'd take a lot of work to implement.
but maybe not to polyfill using proxies [1][2]. I wonder how far away the polyfill is from the latest spec draft.
Tests for the polyfill could feed the standard tests and the native implementation tests. Your code wouldn't need to change the day the standard version comes along (unless major changes to the spec happens, but this seems unlikely)


[1] https://mail.mozilla.org/pipermail/es-discuss/2012-December/026772.html
[2] https://github.com/tvcutsem/harmony-reflect/blob/master/examples/observer.js

Comment 11

[Brandon Benvie [:benvie]]

(In reply to David Bruant from comment #10)
> (In reply to Brandon Benvie [:bbenvie] from comment #8)
> > (In reply to Victor Porof [:vp] from comment #2)
> > > (In reply to David Bruant from comment #1)
> > > 
> > > Strongly agree with using Object.observe or Object.watch.
> > 
> > Object.observe is of course much preferable to this, but implementation is
> > probably far off since no one has even begun work on it and it seems like
> > it'd take a lot of work to implement.
> but maybe not to polyfill using proxies [1][2]. I wonder how far away the
> polyfill is from the latest spec draft.
> Tests for the polyfill could feed the standard tests and the native
> implementation tests. Your code wouldn't need to change the day the standard
> version comes along (unless major changes to the spec happens, but this
> seems unlikely)

I was thinking about that as well. I think you could probably implement the whole spec using proxies. Hell, you could pretty nearly implement the whole spec using dirty checking. It seems like it might be worth doing.

Comment 12

[Paul Rouget [:paul]]

Attachment: Patch v1

. Renamed ObjectEmitter to ObservableObject
. Moved in its own file
. Better test coverage
. Fixed defineProperty

Comment 13

[Paul Rouget [:paul]]

Attachment: Patch v1.1

Fixed the way event-emitter.js is imported.

Comment 14

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=ed12defccb13

Comment 15

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/8ead44298a4c

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ead44298a4c