jsfiddle / togetherjs

A service for your website that makes it surprisingly easy to collaborate in real-time.
https://togetherjs.com
Mozilla Public License 2.0
7.01k stars 850 forks source link

Deferred function called in pagehide event handler is not executed #885

Closed pindexis closed 9 years ago

pindexis commented 11 years ago
  window.addEventListener("pagehide", function () {
    storage.tab.set("peerCache", serialize());
  }, false);

storage.tab.set is an async function (because of the setTimeout function) and therefore will not be executed because the page change before changes take place.

ianb commented 11 years ago

Ah, good catch. I wanted storage to be async so that an async storage system could be swapped in later (e.g., IndexedDB, or some iframe/postMessage system). I think typically such systems might well work fine in this case, because the request to store data would have been added to the queue, and while we'd never get confirmation of the save working it should happen. But in this case we aren't doing the work until later.

I guess one approach would be to do the work immediately, but still resolve the Deferred in the setTimeout() block. Also I'm not sure if Deferred itself resolves in an async fashion...? Technically Promises/A+ are not supposed to resolve synchronously, but I have no idea if jQuery.Deferred guarantees that. (The code for Deferred is also a bit obtuse.)

pindexis commented 11 years ago

yes, I think doing the work immediately and still return a deferred object would be a good approach for simulating an async behavior, something like :

    function () {
      //do work here
      return Deferred(function (def) {
        setTimeout(util.resolver(def, function () {
            // nothing here !
        });
      });
    }

by Deferred being resolved synchronously, do you mean that calling deferred.solve will trigger the onFulfill/onReject callbacks immediately? i can't see the impact of that if setTimeout is present.

ianb commented 11 years ago

So, we could just do:

return Deferred(function (def) {
  ... do work ...
  def.resolve();
});

I worry that this could cause callbacks to be called immediately (though according to Promises/A+ they aren't supposed to be), and then that there will be a substantial logic change if we have a "real" async backend.

This is actually sufficient:

return Deferred(function (def) {
    ... do work ...
    setTimeout(def.resolve);
});

because I happen to know that this doesn't have to be set properly for the def.resolve method to work. But that might just be an accident of how jQuery.Deferred is implemented.

pindexis commented 11 years ago

ah, now I've got it, thanks.

pindexis commented 11 years ago

I faced some issue when changing clear method because it calls keys function which is asynchronous, I think of duplicating code but there might be a better approach.