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

Add support for this.once #185

Closed
necolas opened this issue Oct 16, 2013 · 14 comments
Closed

Add support for this.once #185

necolas opened this issue Oct 16, 2013 · 14 comments

Comments

@necolas
Copy link
Contributor

necolas commented Oct 16, 2013

To use jQuery's .once method.

@angus-c
Copy link
Contributor

angus-c commented Oct 16, 2013

easy way is to add a function wrapper called once in utils.js

so then this.on('click', utils.once(myHandler))

Tal expressed interest in doing this

@tgvashworth
Copy link
Contributor

I'm interested in why this isn't baked in. I totally respect the decision not to have it, but I'm just wondering what the reasoning is?

Also, what are the pros and cons of, for example, using a withOnce mixin?

@necolas
Copy link
Contributor Author

necolas commented Oct 16, 2013

I feel like this.once() is more intuitive than something like a utility.

@angus-c
Copy link
Contributor

angus-c commented Oct 16, 2013

The overhead of adding fully fledged this.once is we would need registry
and logger to have new counterparts to match. With a once(fn) wrapper we
could add a re-usable util AND get this.once type behavior at very little
cost and preserve the simple trigger/on/off event troika

Having a withOnce mixin would be saying we don't want it baked into core. I
mean again its not wrong but idf we were to have a fully fledged this.once
(as opposed to my once(fn) util suggestion) then we may as well add it
directly to core.

@angustweets

On Wed, Oct 16, 2013 at 2:03 PM, Tom Ashworth notifications@github.comwrote:

I'm interested in why this isn't baked in. I totally respect the decision
not to have it, but I'm just wondering what the reasoning is?

Also, what are the pros and cons of, for example, using a withOnce mixin?


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-26458056
.

@angus-c
Copy link
Contributor

angus-c commented Oct 16, 2013

Intuitive because jQuery does it? Seems like a slightly gratuitous
extension of the API when really all we want is a new flavour of on.

trigger/on/off is a really simple event API and I'd prefer to keep it at
that if we can. If utils.once(fn) proves gnarly then sure lets do this.once
instead

@angustweets

On Wed, Oct 16, 2013 at 2:09 PM, Nicolas Gallagher <notifications@github.com

wrote:

I feel like this.once() is more intuitive than something like a utility.


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-26458614
.

@necolas
Copy link
Contributor Author

necolas commented Oct 16, 2013

I see. Thanks for the explanation!

@tgvashworth
Copy link
Contributor

Ok, I see – but just to be clear, I wasn't suggesting Flight ship with a withOnce, I was just suggesting it as an alternative to a util. Or was that clear, and I've missed what you meant?! :)

@angus-c
Copy link
Contributor

angus-c commented Oct 16, 2013

Ah I see! Yes, I got the wrong end of the stick.

Still I think utils.once(fn) could be a useful util in general - and we
already have a utils.js module with several function wrappers in this vain
so its probably easier / less surprising to just put it there

@angustweets

On Wed, Oct 16, 2013 at 2:23 PM, Tom Ashworth notifications@github.comwrote:

Ok, I see – but just to be clear, I wasn't suggesting Flight ship with a
withOnce, I was just suggesting it as an alternative to a util. Or was
that clear, and I've missed what you meant?! :)


Reply to this email directly or view it on GitHubhttps://github.com//issues/185#issuecomment-26459980
.

@psyduk
Copy link

psyduk commented Oct 16, 2013

i made a pull request to implement this feature into utils. it is similar to the lodash/underscore implementations.

@giuseppeg
Copy link
Contributor

While I think that is good to have an utility like once, I would like to point out that a such method doesn't remove event handlers after the first execution of the fn.
By the way jQuery calls it one.

My proposal: https://gist.github.com/giuseppeg/7020843

@angus-c
Copy link
Contributor

angus-c commented Oct 17, 2013

@giuseppeg why does it need to? In fact isn't it cleaner to remove all handlers in one go at teardown?

Moreover your solution is incomplete, we'd also need to add one advice methods to registry and logger. IMO keeping all binding in a single API (on) causes less confusion all around and keeps our codebase small. Also one and once are both very unintuitive names for an event binder - they don't suggest binding at all (just because jQuery did it doesn't make it right)

Let's try and keep our API simple. The following is intuitive and adds nothing to our core code base (other than a reusable util)...

this.on('click', utils.once(myHandler))

@giuseppeg
Copy link
Contributor

@angus-c because it keeps listening for that event and handling it when we actually don't need it anymore.

That said, I am fine with the util solution :)

@psyduk
Copy link

psyduk commented Oct 17, 2013

I think this fits in nicely with the "util" idea. It provides something that underscore or lodash provides without having to actually including the libraries.

@tgvashworth
Copy link
Contributor

I think this can be closed, as it got added in #186.

@angus-c angus-c closed this as completed Dec 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants