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

Create withProxy mixin #1

Open
tgvashworth opened this issue Jan 29, 2015 · 7 comments
Open

Create withProxy mixin #1

tgvashworth opened this issue Jan 29, 2015 · 7 comments

Comments

@tgvashworth
Copy link
Contributor

Flight v2 will remove event proxying from core, so this functionality needs to be replaced by a mixin.

There are a couple of possible APIs:

  • this.(un)proxy which would entirely separate proxying from the normal event-binding flow. This is preferential in my opinion but isn't backward compatible.
  • wrap this.on (and this.off) to intercept proxy-looking arguments. This isn't a good solution because of the complexity in getting the Flight registry to behave correctly in teardown situations.
@ahume
Copy link
Collaborator

ahume commented Feb 1, 2015

First API is preferable in my view. Those wanting to use v2 with backwards compatible event proxying can provide their own advice around this.on/off.

A new API also gives scope for new stuff, maybe the data transform features of with-rebroadcast?

var transform = function (e, data) {
    return {
        value: data.value,
        anotherValue: 'special extra value'
    }
}
this.proxyEvent('sourceEvent', 'targetEvent', transform);

@giuseppeg
Copy link

Yeah the first API looks better to me too.
ideas:
this.proxyEvent(true/false, ...)
or
this.proxyEvent.on()
this.proxyEvent.off()

@ahume
Copy link
Collaborator

ahume commented Feb 2, 2015

I like the idea of proxyEvent.on/off as it apparently leaves the original API intact, but simply moved to the mixin.

@ahume
Copy link
Collaborator

ahume commented Feb 2, 2015

Since we’re rebuilding this, a good opportunity to think about it’s scope. I'm not 100% sure what the behaviour is in core (and the tests are lacking), but here’s some things this could do out of the box.

// Simple 1:1 proxy.
this.eventProxy.on('sourceEvent', 'targetEvent');
this.eventProxy.off('sourceEvent', 'targetEvent');

// Aggregate different event types into a common type.
this.eventProxy.on('sourceEvent1 sourceEvent2', 'targetEvent');
this.eventProxy.off('sourceEvent1', 'targetEvent'); // Can remove one or both at a time.

// Third argument when true cancels propagation of sourceEvent above this.node.
this.eventProxy.on('sourceEvent', 'targetEvent', true);

// Third (or fourth) argument takes a function for transforming event data in the
// new event (see https://github.com/ahume/flight-with-rebroadcast).
this.eventProxy.on('sourceEvent', 'targetEvent', transformFunction);

// Selector lookups in attributes, plus combination of the above options.
this.eventProxy.on('click', {
  button: 'formSubmit',
  expand: 'expandForm'
}, true, transform);

// Remove them individually or as a group.
this.eventProxy.off('click', {
  button: 'formSubmit',
  expand: 'expandForm'
});

// Passing third/fourth arguments to `off` does nothing.

I'm not convinced about supporting node/selector as an optional first argument for this. Is there a good use case that can't be solved by listening on this.node for an event to bubble up, and then triggering the proxy on this.node for it to leave the component?

@giuseppeg
Copy link

Is there a good use case that can't be solved by listening on this.node for an event to bubble up, and then triggering the proxy on this.node for it to leave the component?

Maybe a combo of this.on with preventDefault and this.proxy.on can break this

this.on('click', function (e) {
  e.preventDefault();
});

// this.doSomething never invoked
this.on('proxy', this.doSomething);

// proxy is never fired
this.proxy.on('click', 'proxy');

or if you want to proxy clicks on a specific element only.

also we'd either have to document it well or patch the jQuery.event object and its originalEvent - event.currentTarget will always be this.node for example.

@tgvashworth
Copy link
Contributor Author

Hopefully @ahume can provide an update on this, but we've come up with a (neat, I think) abstraction over proxying and rebroadcasting that will allow both to happen in a composable way, and then we can come up with API sugar over that — for example, this.proxy('x', 'y') will just be sugar over an event type change.

@ahume
Copy link
Collaborator

ahume commented Feb 8, 2015

Update is https://github.com/flightjs/flight-with-event-proxy which implements the core of what @PhUU describes above.

Let's raise further design questions/issues on that repo.

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

3 participants