jdarling / Object.observe

Object.observe polyfill/shim trying to be compliant
The Unlicense
229 stars 32 forks source link

Object.deliverChangeRecords not available #13

Open frostney opened 10 years ago

frostney commented 10 years ago

To deliver the change records, this polyfill uses the following API:

var obj = {};
var changeHandler = function(changes) {
  console.log(changes);
};

Object.observe(obj, changeHandler);
obj.test = 5;

Object.getNotifier(obj).deliverChangeRecords();

In regards to the specification (http://wiki.ecmascript.org/doku.php?id=harmony:observe_public_api) and with the latest Chrome stable (36), delivering the change records actually works like this:

var obj = {};
var changeHandler = function(changes) {
  console.log(changes);
};

Object.observe(obj, changeHandler);
obj.test = 5;

Object.deliverChangeRecords(changeHandler);

Is there any chance that Object.deliverChangeRecords will make it into this polyfill?

jdarling commented 10 years ago

Object.deliverChangeRecords is a pretty big change in the current structure of the code but it could be implemented. As I don't know when I will have time to actually do it I would suggest creating a pull request and posting it here for review/merge.

I'll keep this open till either someone creates a PR or until I have time to do the updates myself. As I said, I'm not sure when that will happen though.

ghost commented 10 years ago

Well, now I’m confused. Up until now I thought Object.deliverChangeRecords is meant for immediate delivery. But both Chrome 36 and Chrome Canary act exactly the same way regardless of presence of Object.deliverChangeRecords. Which means not only interface in the polyfill is different, but so is the behaviour as well. Is there a test Chrome actually passes I should look into?

jdarling commented 10 years ago

If there is, I haven't seen it. Problem is the spec for deliverChangeRecords has changed several times :(

If you read the spec (as its written now) deliverChangeRecords basically queues up the changes and processes them as soon as possible. Not guaranteed delivery immediate, but sooner...

You can see in the polyfill today that it used to take an object and deliver the changes based on that object, now it seems it should deliver all changes for all objects being watched to whatever handler you pass in. This seems... Odd... at best.

jdarling commented 10 years ago

@klimlee I made you a contributor on this project since really you have been keeping it up to date way more than I have. Hopefully this way you can post code directly instead of issuing PR's :)

ghost commented 10 years ago

Thanks, hopefully yes; though right now I’m working on a change I would actually like you to review.

Odd... at best

Yeah, I cannot imagine a use case for that current deliverChangeRecords in the first place.

Not guaranteed delivery immediate, but sooner

The problem is that it doesn’t seem to work in Chrome at all. Its implementation is actually pretty easy to comply with: Object.deliverChangeRecords = function() {};, but I doubt that was their intention :)

So for now I’m inclined to wait for the spec to clarify.

jdarling commented 10 years ago

Just checked the Spec again about this, and here is what it has to say:

Object.deliverChangeRecords
A new function Object.deliverChangeRecords(callback) is added, which behaves as follows:

If IsCallable(callback) is not true, throw a TypeError exception.
Call [[DeliverChangeRecords]] with arguments: callback repeatedly until it returns false (no records were pending for delivery and callback no invoked).
Return.

No example of usage yet, but if I read that correctly then one could assume a usage something like:

var obj = {};
var changeHandler1 = function(changes) {
  console.log('1)', changes);
};

var changeHandler2 = function(changes) {
  console.log('2)', changes);
};

Object.observe(obj, changeHandler);
obj.test = 5;

while(Object.deliverChangeRecords(changeHandler2)){
  console.log('got a change')
}

In the above the the while loop would execute once and only once (since there was only one change), and would call the callback changeHandler2 once with the change record.

What (still) can't be assumed is if changeHandler1 would or would not be executed as well. Wonderful blank area for the spec :(

ghost commented 10 years ago

Your code looks similar to what I’ve been trying to run in Chrome when reported it didn’t work. And, surprisingly, it’s a wrong idea of how deliverChangeRecords works.

It’s becoming more and more strange. I’ve finally managed to get it in Chrome. Guess what? It only works if the handler you pass into deliverChangeRecords is already observing the target. So the same handler would be executed either synchronously with deliverChangeRecords or asynchronously without. Answering your question, any other handlers would be executed as is, so in your case changeHandler would still be called asynchronously, independently of changeHandler2.

But I still can’t make any sense of that. Wouldn’t it be more logical to call some handler synchronously on a specific object, not on the global? Calling an arbitrary function instead of an existing handler, not use a completely separate syntax to change the callback’s mode? Or allowing some sort of synchronous observation per se instead of synchronous delivery? Given the fact we both misunderstood the spec the same way, I’d say there’s something wrong with it (assuming there is nothing wrong with us).

I’m not sure I’m ready to implement, since, even though I now know how it works, I can’t understand why.

jdarling commented 10 years ago

That does make it even more odd. So basically deliverChangeRecords is supposed to deliver only associated (already being observed) changes to the specific handler...

Yep I not only think this makes 0 sense but that it's something I should probably send out on the mailing list and get clarification on. Will follow up with that later tonight.

Oh, and I agree, it isn't worth implementing at this time.

jdarling commented 10 years ago

Found more info and got a response on the list :)

http://wiki.ecmascript.org/doku.php?id=harmony:observe

Reading through the New Internal Objects, Properties, and Algorithms and Modifications to Existsing Algorithms sections makes the change a lot more clear.

deliverChangeRecords basically forces a synchronous delivery of changes associated with listener (f) and (f) only. Any objects that (f) is listening to's changes would be delivered upon calling Object.deliverChangeRecords(f). The internal DeliverChangeRecords for O would be called with an argument of (f) allowing for delivery of only those.

We could actually use the existing setup (option 1) iterating over all known observed objects and calling getNotifier(O).deliverChangeRecords(f) causing an O(n) call length. (option 2) We could implement a hash (or WeakMap if available) that provides (f)->[O] mappings and then iterate only over [O] with getNotifier([O]).deliverChangeRecords(f) at the cost of additional memory. (option 3) Or we could go back and implement the new spec catching some of the other new stuff as well like Synthetic Change Records.

Knowing how much time I have I think Option 1 first then Option 3.

Klimlee, I know you are already working on some changes in the code, what is your opinion and what changes were you working on?

ghost commented 10 years ago

It’s still not clear to me why we want to force delivery to specific listeners but not on specific objects.

1, but if 2 is just 1 + hash, then 2. WeakMap won’t help; the further I go, the more I understand we’ll always be stuck with at least one retainer (which is what’s still keeping me awake). Option 3 sounds like refactoring to me (not saying we shouldn’t consider that, rather that I’m certainly not brave enough to do it on my own :)

jdarling commented 10 years ago

Completely agree, I can't come up with a valid usecase for only delivering messages to one handler and not the others, but who am I to argue :). Guess maybe a logging endpoint type situation is the only real example I can come up with.

Yeah, 3 is a refactor. Its been a long time since I refactored the code to match up to the spec and there is quite a bit out of sync. Though if it were something to take on it starts to make sense to introduce Gulp into the equation and separate out all of the internal classes into their on libraries then build the final Object.observe.poly.js file. Really quite an undertaking.

Will have to put some nogging on it, but right now I'm buried in other work :(

ghost commented 10 years ago

right now I'm buried in other work

Same here. Plus memory issues are the most important dot on my radar for now. Until it’s solved the poly is not safe for large scale apps. I’m not sure it’s possible, but I still hope there is something I don’t understand and eventually will.