Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make DOMRequest inherit from DOMFuture #19

Open
DavidBruant opened this issue Feb 6, 2013 · 11 comments
Open

Make DOMRequest inherit from DOMFuture #19

DavidBruant opened this issue Feb 6, 2013 · 11 comments

Comments

@DavidBruant
Copy link
Contributor

We're seeing the proliferation of DOMRequests in current specs. In recent example is the alarm API.
Discussion is probably needed, but I feel it's unlikely the FirefoxOS folks will move away from DOMrequests, because a lot of Gaia code relies on them.
A way forward is to make DOMRequest inherit from DOMFuture and ask everyone to move away from DOMRequests (or creating equivalents)

@slightlyoff
Copy link
Owner

Send a reformed API pull request!

@annevk
Copy link
Collaborator

annevk commented Feb 7, 2013

It would probably be good to elaborate a bit on why DOMRequest cannot be used instead (given we expand it with features such as then()).

@mounirlamouri
Copy link
Contributor

No, we might move away from DOMRequest if there is a very good proposal that beats DOMRequest. DOMFuture in its current form is more or less DOMRequest with a then() method.

@DavidBruant
Copy link
Contributor Author

@mounirlamouri Glad to learn Moz could move away from DOMRequest :-)
What are the criteria that makes a DOMRequest competitor a "very good proposal"?
You make sound like "with a then() method" is a mere addition, but I have to disagree. Chaining and automatic error forwarding make a huge difference from a JS dev standpoint. And obviously readability. Taking the example of the FileHandle API:

var idbreq = indexedDB.open("MyTestDatabase");
idbreq.onsuccess = function(){
  var db = idbreq.result;
  var handleReq = db.mozCreateFileHandle("test.bin", "binary");
  handleReq.onsuccess = function(){
    var handle = handleReq.result;
    var lockedFileReq = handle.open('readwrite');
    lockedFileReq.onsuccess = function(){
      var lockedFile = this.result;
      // now I can start playing with my file
      // ... which will generate more DOMRequest and indentation levels
    }
  };
};

With Future:

var idbreq = indexedDB.open("MyTestDatabase")
  .then(function(db){
    return db.mozCreateFileHandle("test.bin", "binary");
  })
  .then(function(handle){
    return handle.open('readwrite');
  })
  .then(function(lockedFile){
    // play with the lockedFile
  })
};

About error handling, for most practical purposes, one wants to know if an error happened somewhere in one of the steps. In that case, one just add a final catch ( #21 but that's just sugar for .then(undefined, onreject)).The DOMRequest case requires to put test in every intermediate functions and branch on whether to procede. With future, the "branching" is built-in in the "future protocol"

Reading over Gaia's code, in a lot of cases, functions take an extra callback parameter which wouldn't be necessary if the function returned a future. It results in code creating functions everywhere.
I've recently come across a case that demonstrates the verbosity of callback based APIs by comparison with promise/future APIs: https://gist.github.com/DavidBruant/4713731

@mounirlamouri
Copy link
Contributor

What I meant is that .then() is easy to add to DOMRequest. Actually, I did that tonight. It allows DOMRequest to be chainable and have .then() waiting for the request to be done if a DOMRequest is returned from the callback. It also go forward in the chain if there is no handler for the result state. It's far from fully done but advanced enough to prove that DOMFuture is mostly naming changes compared to DOMRequest.

@DavidBruant
Copy link
Contributor Author

Sure thing. DOMRequest is very close from being a promise/future already, so I don't think any replacement candidate would be hugely different. Now, it's a bit more than just renaming; the delta from DOMRequest to Future:

  1. DOMRequest have to be construct-able by user code (which includes the accept/reject callback mechanism)
  2. chainable then/catch/done (done is necessary to stop the chain)
  3. add the state attribute
  4. Add a combination mechanism for DOMRequest synchronisation ( Add an equivalent to Q.all #16 )
  5. get rid of the onsuccess/onerror (to not encourage bad patterns)

When looking at the size of the API and the changes I have enumerated, relatively speaking, it is more than just renaming. To the point where we can wonder if the "DOMRequest" name is worth keeping given the amount of make-up? Or should a cat be called a cat?

From a social standpoint, renaming would send a clear signal to everyone involved that something has changed in how the API is expected to be used.

@sicking
Copy link
Contributor

sicking commented Feb 15, 2013

I think we should deprecate DOMRequest in favor of DOMFuture. Whatever DOMFuture ends up looking like, it's better to have a single "promise-like" API than to have two.

We can try to retrofit DOMRequest such that it's mostly or wholly compatible with DOMFuture. If that makes sense or not will depend on what DOMFuture eventually will look like. I personally don't think that DOMFuture should use DOMEvents at all, which would mean that retrofitting DOMRequest essentially means making DOMRequest implement both APIs.

@slightlyoff
Copy link
Owner

My assumption was that we'd repair DOMRequest by doing this:

https://github.com/slightlyoff/DOMFuture/blob/master/reworked_APIs/IndexedDB/after.idl#L51

Keeping the legacy types where we need to but re-casting them in terms of Future -- under the assumption that we'll move to Future for everything new. Or is it the case that these types are more maleable than I think?

@sicking
Copy link
Contributor

sicking commented Mar 8, 2013

Unfortunately some of the IDBRequests that IndexedDB creates fires the "success" events multiple times. In particular the requests returned from the cursor-creating functions openCursor and openKeyCursor.

@sicking
Copy link
Contributor

sicking commented Mar 8, 2013

What might be doable is to create two implementations of the IDBRequest interface. One that also implements the Future interface, and one that also implements the Cursor interface proposed in issue #37

@slightlyoff
Copy link
Owner

Interesting. Yeah, if they're separable contracts, the multiple-success variant could move to being a subclass of IDBRequest (although I don't know how weird that will be).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants