Skip to content

feat(runtime-dom): support event options #149

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

Merged
merged 13 commits into from
Oct 9, 2019

Conversation

CyberAP
Copy link
Contributor

@CyberAP CyberAP commented Oct 7, 2019

There's no support for event options right now, this pull request provides support for event listener options.

You can pass event handler and options as an object:

{
  handler: EventValue,
  options: EventListenerOptions
}

Example:

render() {
  return h('button', {
    onClick: {
      handler: () => { this.counter++ },
      options: {
        once: true
      }
    }
  });
}

I am sure there's something to improve on the TypeScript side, don't hesitate to suggest!

const value =
nextValue && 'handler' in nextValue ? nextValue.handler : nextValue

// go unoptimized route if it has to deal with event options
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a clear idea on how to optimize this yet, but this is definitely too unoptimized - it will cause the event listener to be detached and re-attached on every single component update.

Also need to consider compiled case - for example with @click.cpature we know the options will never change so it should not need to go this route.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a clear idea on how to optimize this yet, but this is definitely too unoptimized - it will cause the event listener to be detached and re-attached on every single component update.

I thought about looseEqual check and thought it would add more overhead than event re-assignment.

Also need to consider compiled case - for example with @click.cpature we know the options will never change so it should not need to go this route.

Could this be a compiler flag?

nextValue = {
  handler: () => {},
  options: {},
  persistent: true
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my surprise looseEqual check is much faster than event re-assignment. So probably not that bad of a tradeoff.

https://jsperf.com/looseequal-or-event-re-assign/1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, as a rule of thumb, anything in pure JavaScript will be faster than code that touches the DOM.

@CyberAP CyberAP requested a review from yyx990803 October 7, 2019 22:17
Copy link
Member

@yyx990803 yyx990803 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the implementation looks good now. Would you be interested in adding tests for this module? I think we can directly test the patchEvent function (instance can just be null since it's only used for error handling and that will be tested in error handling integration tests)

@CyberAP CyberAP requested a review from yyx990803 October 9, 2019 18:39
@yyx990803 yyx990803 merged commit 08df965 into vuejs:master Oct 9, 2019
@yyx990803
Copy link
Member

👍 great work!

@vue-bot
Copy link
Contributor

vue-bot commented Oct 9, 2019

Hey @CyberAP, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚

sxzz added a commit that referenced this pull request Aug 12, 2024
Co-authored-by: 三咲智子 Kevin Deng <[email protected]>
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