Closed Bug 898394 Opened 6 years ago Closed 6 years ago

Implement an Object Emitter

Categories

(DevTools :: General, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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
(In reply to David Bruant from comment #1)

Strongly agree with using Object.observe or Object.watch.
Attached patch Patch v0.1 (obsolete) — Splinter Review
Proxy. Based on Benvie's code. Test are coming (I hit a clobber in the middle of my work).
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #781672 - Flags: review?(vporof)
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;
  }
Attached patch patch v0.2Splinter Review
with basic tests.
Attachment #781672 - Attachment is obsolete: true
Attachment #781672 - Flags: review?(vporof)
Attachment #781687 - Flags: review?(vporof)
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?
Attachment #781687 - Flags: review?(vporof) → review+
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.
Blocks: 895360
(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.
(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.
(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
(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.
Attached patch Patch v1 (obsolete) — Splinter Review
. Renamed ObjectEmitter to ObservableObject
. Moved in its own file
. Better test coverage
. Fixed defineProperty
Attached patch Patch v1.1Splinter Review
Fixed the way event-emitter.js is imported.
Attachment #783029 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/8ead44298a4c
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8ead44298a4c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25
See Also: → 1234880
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.