Re: [File API] events vs callbacks (was: Re: New FileAPI Draft | was Re: FileAPI feedback)

On Fri, Aug 14, 2009 at 5:27 PM, Garrett Smith<dhtmlkitchen@gmail.com> wrote:
> On Fri, Aug 14, 2009 at 3:49 PM, Jonas Sicking<jonas@sicking.cc> wrote:
>
>> On Fri, Aug 14, 2009 at 11:09 AM, Garrett Smith<dhtmlkitchen@gmail.com> wrote:
>>> On Mon, Aug 10, 2009 at 6:48 PM, Jonas Sicking<jonas@sicking.cc> wrote:
>>>> On Sat, Aug 8, 2009 at 9:37 AM, Garrett Smith<dhtmlkitchen@gmail.com> wrote:
>>>>> On Thu, Aug 6, 2009 at 11:31 AM, Jonas Sicking<jonas@sicking.cc> wrote:
>>>>>> On Wed, Aug 5, 2009 at 4:26 AM, Anne van Kesteren<annevk@opera.com> wrote:
>>>>>>> On Wed, 05 Aug 2009 10:04:28 +0200, Arun Ranganathan <arun@mozilla.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> In the case of file read APIs, simply getting the data asynchronously is
>>>>>>>> more convenient than using events.  There is no intrigue at work here,
>>>>>>>> merely disagreement.
>>>>>>>
>>>>>
>>>>> Who said "getting data asynchronuosly" is less convenient than "using
>>>>> events"? I am not sure what that really means, but I am pretty sure
>>>>> this is not a debate of "using events" vs. "getting data
>>>>> asynchronously".
>>>>>
>>>>> Arun also argued previously: "Callbacks are used so that these APIs
>>>>> can behave asynchronously." The callback parameter has nothing
>>>>> whatsoever to do with the API being asynchronous. How the callback is
>>>>> registered is a design decision.
>>>>>
>>>>> It was determined and agreed that reading a file should be
>>>>> asynchronous. We should all be on the same page now.
>>>>>
>>>>> Event callbacks are asynchronous. So, it is not a matter of arguing
>>>>> "synchronous events" with "asynchronous callback".
>>>>>
>>>>> Problems with the existing design were already explained in the other
>>>>> thread. Some of those problems were not acknowledged by the author.
>>>>> There is not a consensus.
>>>>>
>>>>> I would like to register a Formal Objection.
>>>>>
>>>>> The File API design does not provide for standard callback
>>>>> registration, but instead uses its own callback registration
>>>>> mechanism. This limits the flexibility of implementation code and
>>>>> limits extensibility of the API.
>>>>>
>>>>>>> I could imagine that for reading data you might want to have events though
>>>>>>> so that in the future we can introduce progress events if that is found
>>>>>>> necessary. E.g. if the actual file is not on the same computer as where the
>>>>>>> user selected it.
>>>>>>
>>>>>
>>>>> An API should generally be extensible.
>>>>>
>>>>> Problem: The File API couples the call to read with the callback.
>>>>> This violates OCP.
>>>>>
>>>>> This is easily achievable by decouple the call to "read" from the
>>>>> notification that the reading has completed.
>>>>>
>>>>> The way to solve this problem entails the reader implementing either:
>>>>>  1) DOM Events
>>>>>  2) DOM Event handler properties
>>>>
>>>> Skipping this part in order to get to the meat of the discussion which
>>>> is the actual proposal. I hope that this is ok. I assure you I read
>>>> all of the above.
>>>>
>>>>>> Do you have a proposal for what this would look  I'm not excited
>>>>>> about creating something that's significantly more complex than the
>>>>>> current API.
>>>>>>
>>>>>
>>>>> Removing the callback parameter and using a Reader was proposed in
>>>>> that other thread. While it was argued that it had fewer LOC in the
>>>>> simplest cases, it wasn't ever demonstrated as being more complicated.
>>>>>
>>>>> The current possibility is to have just a callback:-
>>>>>
>>>>> file.getAsDataURL(handleURL);
>>>>>
>>>>> - and then check the status to see if it was successful.
>>>>>
>>>>> How to implement multiple callbacks with that?
>>>>>
>>>>> Multiple callbacks can be implemented with the current draft proposal,
>>>>> e.g. ala "file.read( callWhenDone )", but not without a good amount of
>>>>> work. It would entail the program to create its own Reader that is
>>>>> itself an EventPublisher[1] Depending on the situation, that could be
>>>>> elegant or could be extra plumbing.
>>>>
>>>> I so far have not seen any use case that required the need for
>>>> multiple callbacks, or indeed where multiple callbacks would even be
>>>> useful. The use case from the previous thread seemed to be to hide UI
>>>> once file loading was done, no matter if the load succeeded or not.
>>>> The easiest way to do that seems to be:
>>>>
>>>> file.getAsBinary(myHandler);
>>>>
>>>> function myHandler(data, error) {
>>>>  ... hide UI ...;
>>>>
>>>>  if (error) {
>>>>    ... handle error ...
>>>>    return;
>>>>  }
>>>>
>>>>  ... process data ...
>>>> }
>>>>
>>>
>>>
>>>
>>> Regarding the callback function you've written, one parameter should
>>> be all that is necessary.
>>>
>>> - function myHandler(data, error)
>>> + function myHandler(ev) {
>>>    ...
>>>    if(ev.readFailure) {
>>>       ...
>>>    } else {
>>>        var data = ev.data;
>>>    }
>>> }
>>>
>>> I disagree that adding if-else's to the callback is better. It gives
>>> the callback more generic functionality.
>>>
>>> The added conditional logic increases the indentation and makes it
>>> harder to see what the method is doing. Testing that callback is going
>>> to require many tests, to cover all conditions. Now it's a toy
>>> example, but imagine the function gets longer (as functions tend to
>>> do) and becomes something like 50 LOC and has more indentations. Isn't
>>> it better to have a couple of clearly defined methods that follow SRP?
>>>
>>>> If we used a reader (as proposed below) together with progress events,
>>>> you'd get code like:
>>>>
>>>> reader = getAReader();
>>>> reader.onerror = function() {
>>>>  ... handle error ...
>>>> }
>>>> reader.onload = function() {
>>>>  ... process data ...
>>>> }
>>>> reader.onloadend = function() {
>>>>   ... hide UI ...
>>>> }
>>>> reader.read(file);
>>>>
>>>
>>> Only where one wanted it. If you read what I wrote:-
>>>
>>>  var reader = getAReader(); // etc.
>>>  reader.oncomplete = readDone;
>>>  reader.read( file );
>>>
>>> - there is a reader with oncomplete only. But then, if the author is
>>> having conditional logic inside the complete handler to determine if
>>> there was an error, if there was success, failure, etc. Those
>>> conditions might best be moved elsewhere, so that each method
>>> unambiguously does one thing, not looking like a sea of indentation of
>>> if-else's. Much easier to read and follow simple code like that and it
>>> will be much much easier to test and refactor because it is clean and
>>> decoupled.
>>>
>>> In the callback parameter example you posted and Reader example I
>>> posted, both could be refactored to have a readDone method as:-
>>>
>>> function readDone(ev) {
>>>    if(ev.isError) {
>>>        handleError(ev);
>>>    } else (
>>>        handleSuccess(ev);
>>>    }
>>> }
>>>
>>> (provided an "isError" property on the payload).
>>>
>>> The problem with the callback-parameter design is when the program
>>> using that API is designed in a way where objects are decoupled. One
>>> object in that program, call it UIMessage, wants to know when read is
>>> file.complete because it needs to handle removing the UI busy
>>> indicator. Another object in that program, DataTable (etc), wants to
>>> listen for file.success, so it can get the file content payload.
>>> DataTable and UIMessage know nothing about each other, only listen for
>>> events.
>>>
>>> * UIMessage object that wants to listen to complete from the read and
>>> that takes care of messaging ONLY.
>>>
>>> * a JSONParser listens for SUCCESS
>>>
>>> In a simple example, only one callback is fine.
>>>
>>> For an application that wants multiple callbacks, the strategy is either:
>>> Current API, callback param:
>>> 1) write spaghetti
>>> 2) write an event publishing adapter for file reads
>>>
>>> API that supports events
>>> 1) use multiple callbacks.
>>>
>>> Now what is interesting is the inception of an abstraction, such as
>>> UIMessage. In a simple case, it might be a function
>>> "showMessage(msg)". As the site evolves, there may be cases where the
>>> function is extended to handle different types of messages and
>>> messages that appear in different places. Now we have:-
>>>
>>> showMessage(msg, parentElementId, className);
>>>
>>> function showMessage is gets to the point where it wants to be a
>>> reusable object that encapsulate all the variance. The logic in the
>>> if-else's gets extracted and moved to methods.
>>>
>>> The proliferation of broken and badly designed garbage of the web
>>> (AISI) does not serve as an example of what works. It should be an
>>> example to learn from. A dynamic language like javascript makes it
>>> really easy to make an awful mess. However, the flip side of the coin
>>> is that a dynamic language like javascript makes it really easy to
>>> make flexible APIs. All the client of a Reader would want to know is:-
>>> 1) Can it read the file?
>>> 2) Can I listen for success/failure/complete?
>>>
>>>> I can't really say that this seems like a big benefit.
>>>
>>> The benefit is that I can have more than one callback if I need it. It
>>> is sometimes needed.
>>>
>>>>
>>>>> The proposed Reader solution:
>>>>>
>>>>>  var reader = getAReader(); // etc.
>>>>>  reader.oncomplete = readDone;
>>>>>  reader.read( file );
>>>>>
>>>>> This allows new features to be added, allowing forwards changes
>>>>> without changing existing features.  This also has a benefit of
>>>>> providing an Interface (Reader) that 1) reads, and 2) fires events.
>>>>> The reader could be any sort of reader.
>>>>
>>>> What do you mean by "The reader could be any sort of reader"?
>>>>
>>>
>>> Any object that has a read method and implements the events of Reader.
>>> The client does not need to care what sort of reader it is behind the
>>> scenes, so long as it is capable of
>>> 1) read( file )
>>> 2) firing callbacks
>>>
>>> A factory would be a simple way for that. That is why I had a
>>> "getAReader()" in the example that you read. Lets not expose a bunch
>>> of constructors; just a method to get a reader. I can't think of
>>> another type of Reader I'd want. For JSON, I could use the forthcoming
>>> JSON.parse. For CSV, I could probably use fileContents.join(",").
>>>
>>>>> Using a Reader decouples the type of read from the file object.
>>>>>
>>>>> The other proposed solution was to have the events on the file, not as
>>>>> a separate Reader.
>>>>>
>>>>> file.oncomplete = contentsRead;
>>>>> file.readAsDataUrl();
>>>>> file.readAsText();
>>>>>
>>>>> That puts the context on the file. This means the callback must check
>>>>> the result of the type of read. In the example above, "contentsRead"
>>>>> would have to know something about the data read (is it a URL or
>>>>> Text?) and would end up having "if else" statements that call other
>>>>> functions. This was provided in Jonas' example of reading file raw
>>>>> data and as a data-url.
>>>>
>>>> I'm strongly against this design. See my recent post at
>>>> http://lists.w3.org/Archives/Public/public-webapps/2009JulSep/0547.html
>>>> for why. Basically using the same object to both represent a file
>>>> resource, as well as a read-request from that resource seems very
>>>> prone to bugs.
>>>>
>>>
>>> I can see that. The same is also true for file.getAsURL( handler ). It
>>> puts a lot of responsibility on the File. Really, that reading
>>> behavior does not belong on the file. I think of the file as being a
>>> simple object that has a few properties (size, name, etc). It seems
>>> wrong to have the File object doing the reading of itself.
>>>
>>> The Reader does only reading. This means that if the read varies from
>>> URI or BINARY or something else (JSON, etc), then a reader suited for
>>> that can read it. The reader reads a file or fails. Calling read(
>>> aFile ) while another read of aFile is in progress should not result
>>> in an error and the caller should not be burdened with checking the
>>> aFile's state.
>>>
>>>>>> / Jonas
>>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Garrett
>>>>> [1] An EventPublisher keeps track of subscribers (other callbacks),
>>>>> provides its own mechanism for registration), and fires events.
>>>>
>>>> / Jonas
>>>>
>>>
>>> Garrett
>>>
>>
>> I think at this point I would like to stop having meta-discussions
>> about which coding pattern is the cleanest one. I think we simply
>> disagree there.
>
> [restored posting order -- to the bottom]
>
> Well I do care very much about clean code, so it is something that
> matters to me. I think API design is relevant to that.
>
> Having one callback with a lot of conditional logic shifts the burden
> of complexity onto the program code. Was there a point to what I was
> saying before about bad APIs encouraging bad implementation code?

I agree that clean code matters. It just seems like we disagree what
is clean code.

>> In order to move forward I think we need to compare actual proposals
>> of APIs and see how well they solve various use cases. As far as I can
>> see we have three concrete proposals:
>>
>> A) The proposal in the current draft [1]
>> B) The proposal from Olli [2]
>> C) The proposal from me [3]
>>
>> I have so far not seen a concrete proposal from you, though what you
>> have described has sounded similar to [3], but with some properties
>> with different names (oncomplete rather than onloadend I think?)
>>
>
> I did not write up IDL, but I did post a proposal:
>
> Create an object that reads and fires events.
>
> var reader = getAReader();
> reader.oncomplete = readDone;
> reader.read( aFile );
>
> function readDone(ev) {
>  // remove loader bar, etc.
> }
>
> I like this way better than IDL, I think IDL is a great second step.
> Can't that still be considered a proposal?
>
> Can you please show me what is wrong with that? I think I like the
> general idea of it.
>
> It allows me to add multiple callbacks without too much hassle of
> creating an EventPublisher for file read events. If EventTarget is
> implemented on that, then native callbacks can be added.

The problem is that it's somewhat lacking in detail. First of all it
seems like you're not using progress events as defined by the Progress
Events draft [4]. Unless you mean that the oncomplete property should
register a listener for some event other than 'complete'. In any case
I can't tell if readDone in the example above will be called only in
success conditions, or in both error and success conditions. I also
can't tell how to read data as binary vs. text vs. data-url.
Additionally in most examples prior to this one you've added "// etc"
at the end of getAReader() which made me unsure if there was
additional syntax being left out.

[4] http://dev.w3.org/2006/webapi/progress/Progress.html

>> The use cases that have been discussed so far are:
>>
>> 1. Reading data from the file in various formats (data-url, binary, text)
>> 2. Progress notifications while reading the file
>> 3. Ability to hide UI once reading is done, weather reading succeeded
>> or not. Do this without having to duplicate code.
>> 4. Ability to pass File object around without risking other users of
>> the file interfering with your reads.
>>
>
> The file is accessible whether or not it is passed. It's in the DOM.
> Am I completely missing something here?

I don't understand what you are saying here.

>> It seems to me that [1] supports 1 and 3.
>
> "[1]" below.
>
> ASIB, givent the current design, multiple callbacks are possible by
> writing an object that publishes events. Basically interpreting the
> success of the file read. Any interested parties listen to the
> FileReader. It would be analagous to the XHR adapter such as Mochikit
> "Deferred". So The current API can do that, but it would be extra work
> to implement in javascript. In cases where a program were growing in
> functionality (can you imagine?) the one callback being fine at first
> and then.
>
> http://www.mochikit.com/doc/html/MochiKit/Async.html

I still have not heard any use case that requires multiple callbacks.
The one use case brought up so far is hiding the UI of a progress bar.
The difference in code here is

reader.addEventListener(..., hideProgressUI, false);
reader.addEventListener(..., function(e) {
  ...process data...
}, false);
reader.read(file);

vs.

text.readAsText(function(data) {
  hideProgressUI();
  ... process data ...
})

while we can argue which is the cleaner code, IMHO the difference is
minor and both solutions solve the use case.

The mochikit Defer object you are referring to, and which allows
multiple callbacks, is not specific to data-loading, but to
asynchronous notifications in general.

> 2 isn't satisfied because
>> there are no progress notifications. It has been suggested (by me and
>> Arun) that this can be fixed, but no proposal has been made yet. 4
>> isn't satisfied because there is a risk that someone else calls
>> cancelReads, which aborts your reads.
>>
>> [2] satisfies 1, 2 and 3. 4 isn't satisfied because if anyone calls a
>> getAsX function, then the events from that read will be intermingeled
>> with your events. Alternatively, if we don't allow parallel reads, if
>> someone else has started a read then you are blocked from reading
>> which means that 4 still isn't satisfied.
>>
>> [3] satisfies 1, 2, 3 and 4, however it is the most complex API out of
>> any API proposed yet (not surprising as that tends to happen when you
>> try to satisfy all use cases :) ).
>>
>
> It seems like a very odd way of thinking about this.

Matching proposals against use cases and requirements is a fairly
common way to design specifications.

>> / Jonas
>>
>> [1] http://dev.w3.org/2006/webapi/FileUpload/publish/FileAPI.html
>> [2] http://lists.w3.org/Archives/Public/public-webapps/2009JulSep/0539.html
>> [3] http://lists.w3.org/Archives/Public/public-webapps/2009JulSep/0565.html
>>
>
> | Above API:
> | reader = new FileReader;
> | reader.readAsBinaryString(myFile);
> | reader.onload = handler;
> | function handler(event) {
> |   doStuffWith(event.target.response);
> | }
> |
> | I'd be interested in feedback from others, especially
> | from people that so far hasn't spoken up.
>
> I read that and it seems good. I've been active on this topic, so I
> took a break and did other stuff. I want to hear what others think,
> too.
>
>> P.S. I assure you I've read your full email below, even though I
>> haven't responded to the individual parts.
>>
>
> You top-replied over everything, taking everything I wrote out of
> context. It totally destroys the discussion and I'm not inclined to go
> retyping stuff. Why would you want to do that?

I was trying to get to the interesting parts of the discussion and
avoid meta-discussions.

For example it's fairly useless to discuss the merits of weather
multiple callbacks or not are required if all proposals that fulfill
the requires support multiple callbacks.

> <funfact>
> I coded for 18 hours yesterday.
> </funfact>
>
> Did I mention that my hands hurt?
>
> Garrett

/ Jonas

Received on Saturday, 15 August 2009 08:13:43 UTC