OpenF2 / F2

Redefining web integration for the financial services community
Apache License 2.0
130 stars 62 forks source link

v2 Contextless Event Unsubscription #156

Closed zackferraro closed 10 years ago

zackferraro commented 10 years ago

In the current working of event unsubscription, if an app registers an event without supplying context (which is allowed behavior), the context defaults to the window. If someone ever does F2.off(null, null, window), it will blow out all the context-less events.

Is this desired behavior?

If not, F2 should use a default context object that lives inside the Events closure.

zackferraro commented 10 years ago

If it is important to apply the handler against the window by default, then F2 can do a check substitution on that argument: var thisContext = (sub.context === defaultContext) ? window : sub.context; sub.handler.apply(thisContext, args);

ilinkuo commented 10 years ago

+1 for the concern about events and the solution.

However, I'm not seeing from the eventEmitter2 api that it can be used in that way F2.off(null, null, window), or emitter.off(null, null, window)?

zackferraro commented 10 years ago

Its in the v2-restructured branch: https://github.com/OpenF2/F2/blob/v2-restructured/src/events.js I don't know much of that branch is staying for v2 (if any), but in effort to learn it so I can better contribute I decided to write some unit tests for it, and this was an issue I couldn't think of a test description for.

montlebalm commented 10 years ago

@zackferraro Thanks for pointing out the flaw. I think we should be able to check the identity of the "context" param and default it to null if it matches window. Later we can default the handler execution context to window if nothing else was provided.

I'll make the change.

markhealey commented 10 years ago

Do we want to add this to #155?

montlebalm commented 10 years ago

@markhealey I don't believe v1 has this problem since it uses EventEmitter. As far as I know, you can't specify the context a handler executes in with anything other than $.proxy or .bind().

markhealey commented 10 years ago

Sorry, my bad—I misread the issue title! Back to regularly scheduled programming...