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

Should work with objects that have their own event system #145

Open
matthewp opened this issue Feb 3, 2017 · 7 comments
Open

Should work with objects that have their own event system #145

matthewp opened this issue Feb 3, 2017 · 7 comments

Comments

@matthewp
Copy link
Contributor

matthewp commented Feb 3, 2017

can-define should work with objects that already have an addEventListener, such as dom elements.

@matthewp matthewp added the bug label Feb 3, 2017
@matthewp
Copy link
Contributor Author

matthewp commented Feb 3, 2017

I'll create a test with an object that has its own mini event emitter system so we can better look at the solution.

@matthewp
Copy link
Contributor Author

matthewp commented Feb 3, 2017

Added a test here: c93641f

@matthewp
Copy link
Contributor Author

matthewp commented Feb 3, 2017

@justinbmeyer I did a quick and dirty implementation that uses the element's native addEventListener here: 4fbb7eb

The result of that change is that the DOM events now fire but the property events do not (without that change the opposite happens).

matthewp added a commit that referenced this issue Feb 7, 2017
This makes it possible to use can-define with HTML elements so that both
property events and DOM events can be listened to and fire correctly.

The method for doing this is to call to the existing `addEventListener`
if one exists, otherwise to do the default action of calling to
eventLifecycle.addAndSetup.

Closes #145
matthewp added a commit that referenced this issue Feb 8, 2017
This makes it possible to use can-define with HTML elements so that both
property events and DOM events can be listened to and fire correctly.

This is done by using the can-event/lifecycle/lifecycle mixin, and
passing it our existing addEventListener (where it exists), rather than
using canEvent.addEventListener as lifecycle did before.

Closes #145
@matthewp
Copy link
Contributor Author

matthewp commented Mar 6, 2017

Last I left off with this there was a problem with can-observation expecting 3 argument events.

I patched my local can-observation to fix this:

onDependencyChange: function(ev, newVal, oldVal){
  if(arguments.length === 1 && Array.isArray(ev.args)) {
    newVal = ev.args[0];
    newVal = ev.args[1];
  }

  this.dependencyChange(ev, newVal, oldVal);
},

However, it still fails because there is no batchNum associated with this event:

screen shot 2017-03-05 at 9 12 35 pm

The problem is that I changed can-define to use canEvent.trigger, which triggers real DOM events if it's a DOM element. However this fails because there is no batch stuff happening now.

So I'm not sure how to fix this now. I suppose maybe canEvent.trigger might need to do some batching.

@justinbmeyer
Copy link
Contributor

@matthewp I've been thinking about this probably and how it relates to can-operate.onKeyValue. Some event systems won't have a batchNum. It might be possible to "force" this via the onKeyValue symbol / setup. Can explain more later.

@justinbmeyer
Copy link
Contributor

@matthewp can-operate only expects a onKeyValue that will be called with the new value. We should talk about what this will mean for this effort.

@matthewp
Copy link
Contributor Author

Just noting that this never was fully fixed, should revisit.

@matthewp matthewp removed their assignment Dec 27, 2018
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

2 participants