Skip to content

Adds event modifiers using | character #1641

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

Closed
wants to merge 4 commits into from

Conversation

GarrettGeorge
Copy link

@GarrettGeorge GarrettGeorge commented Aug 9, 2018

Issue Ref: #1088

Uses the pipe character as referenced by Rich here.

Utilizes the 5 options similar to those from Vue listed here

Doesn't break any existing event handlers because the options default to false for all.

Doesn't work with custom events.

@GarrettGeorge
Copy link
Author

GarrettGeorge commented Aug 9, 2018

Shit, forgot to run tests, rookie mistake. I'll rewrite those tomorrow. whoops...

Nvm, it was only 4 tests.

@GarrettGeorge
Copy link
Author

GarrettGeorge commented Aug 9, 2018

Still need to add tests for event modifiers that aren't stop, prevent, capture, once, and passive

  • Invalid modifier test Completed with adfc0e3

@GarrettGeorge GarrettGeorge changed the title Adds event modifiers using | character [WIP] Adds event modifiers using | character Aug 9, 2018
@GarrettGeorge GarrettGeorge changed the title [WIP] Adds event modifiers using | character [Needs Tests] Adds event modifiers using | character Aug 9, 2018
@GarrettGeorge GarrettGeorge changed the title [Needs Tests] Adds event modifiers using | character Adds event modifiers using | character Aug 10, 2018
@Rich-Harris
Copy link
Member

Thanks for this. A couple of things I wonder about and would be interested to hear views on:

  • Should prevent and stop be preventDefault and stopPropagation? It's more to type, but it's arguably clearer
  • How do we deal with IE? It doesn't support the options argument; so this...
node.addEventListener('click', handler, {
  capture: false,
  passive: true,
  once: true
});

...behaves like this:

node.addEventListener('click', handler, {
  capture: true,
  passive: false,
  once: false
});

We have a legacy: true option which could be used here — if a component is compiled in legacy mode, we could generate something like this:

// https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
var passiveSupported = false;

try {
  var options = Object.defineProperty({}, "passive", {
    get: function() {
      passiveSupported = true;
    }
  });

  window.addEventListener("test", options, options);
  window.removeEventListener("test", options, options);
} catch(err) {
  passiveSupported = false;
}

// capture: true
node.addEventListener('click', handler, true);

// passive: true, capture: false
node.addEventListener('click', handler, passiveSupported ? {
  passive: true
} : false);

// once: true
node.addEventListener('click', once(handler), false);

function once(fn) {
  let called = false;
  return function() {
    if (called) return;
    called = true;
    return fn.apply(this, arguments);
  };
}

Separately, it occurs to me that if the prevent/preventDefault modifier isn't used, and event isn't passed into the handler, we could always use passive: true (or is that only beneficial for touchstart and touchmove?).

@GarrettGeorge
Copy link
Author

I was thinking about it and I think stop -> stopPropagation and prevent -> preventDefault is more clear, because if you don't know about stopPropagation() and preventDefault()then stop and prevent will be very unclear to a user.

legacy: true initially seems like the best/easiest option.

I don't have experience with touchStart and touchMove so I'm not entirely sure about the last point. I figure that it wouldn't hurt to check for conditions which implicitly indicate passive: true

@GarrettGeorge
Copy link
Author

GarrettGeorge commented Aug 12, 2018

  • stop -> stopPropagation, prevent -> preventDefault
  • legacy: true for IE
  • passive: true stuff

@lukeed
Copy link
Member

lukeed commented Aug 12, 2018

This is great! I'm definitely a fan of splitting them up like this 🙌

@jacwright
Copy link
Contributor

Hey @GarrettGeorge, any progress on this? Are you still planning on working on it?

@GarrettGeorge
Copy link
Author

I'm still working on legacy and passive, I've just been super busy as of late unfortunately.

@Rich-Harris
Copy link
Member

Closing this in favour of #1819 — thanks for the initial push @GarrettGeorge! Will be great to have this feature available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants