655342 - timer API is awkward

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Dietrich Ayala (:dietrich)]

1. the APIs should return timer objects that have a cancel method, instead of ids.

2. the module should have one cancel method accepts both timer types, instead of separate "clear" methods for each timer type.

actually, fixing #1 might remove the need for #2 altogether, making things yet simpler.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Seems reasonable, as long as we can implement this in a way that doesn't break backward-compatibility.

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(Pushing all open bugs to the --- milestone for the new triage system)

Comment 3

[Erik Vold]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/288

Pointer to Github pull-request

Comment 4

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Dietrich Ayala (:dietrich) from comment #0)
> 1. the APIs should return timer objects that have a cancel method, instead
> of ids.
> 
> 2. the module should have one cancel method accepts both timer types,
> instead of separate "clear" methods for each timer type.
> 
> actually, fixing #1 might remove the need for #2 altogether, making things
> yet simpler.

Do you still think this is necessary ? Timers behave as they do on the web today, I don't think there is a value in introducing new API for doing something that is already a well known. If you need cancel it's easy:

let cancel =  timer.clearTimeout.bind(null, timer.setTimout(function() {}, 0, arg));

Comment 5

[Erik Vold]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] from comment #4)
> (In reply to Dietrich Ayala (:dietrich) from comment #0)
> > 1. the APIs should return timer objects that have a cancel method, instead
> > of ids.
> > 
> > 2. the module should have one cancel method accepts both timer types,
> > instead of separate "clear" methods for each timer type.
> > 
> > actually, fixing #1 might remove the need for #2 altogether, making things
> > yet simpler.
> 
> Do you still think this is necessary ? Timers behave as they do on the web
> today, I don't think there is a value in introducing new API for doing
> something that is already a well known. If you need cancel it's easy:
> 
> let cancel =  timer.clearTimeout.bind(null, timer.setTimout(function() {},
> 0, arg));

It isn't obvious that this would work with IDs returned from setInterval.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 579243 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/288

I'll defer to Irakli on this one.

Comment 7

[Dietrich Ayala (:dietrich)]

i don't remember what code i was writing when i filed this, but it was cumbersome enough for me to file a bug.

i guess if the aim is to mimic web apis and stick to that, then no we don't need this. what are the differences between this and the web timer api?

if there are additive ways to not have to track the ids, that'd be nice to have.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 579243 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/288

Sorry Erik, but I think we should stick to the standard APIs where possible to keep a learning curve minimal. In addition timer id's can be passed across the workers that is not really possible with an objects. Finally we are at the stage where we don't want to make any API changes unless that's necessary which I don't think this is.

That being said it's pretty easy to implement such an API as an utility function if desired:

var timer = require('api-utils/timer');
exports.setTimeout = function setTimeout() {
  let id = timer.setTimeout.apply(timer, arguments);
  return {
    id: id,
    cancel: timer.clearTimeout.bind(null, id)
  };
};
...