-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Conversation
const value = | ||
nextValue && 'handler' in nextValue ? nextValue.handler : nextValue | ||
|
||
// go unoptimized route if it has to deal with event options |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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)
👍 great work! |
Hey @CyberAP, thank you for your time and effort spent on this PR, contributions like yours help make Vue better for everyone. Cheers! 💚 |
Co-authored-by: 三咲智子 Kevin Deng <[email protected]>
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:
Example:
I am sure there's something to improve on the TypeScript side, don't hesitate to suggest!