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

Adds handler for kebab-case event for v-bind:sync; fix #6428 #8297

Merged
merged 4 commits into from
Dec 21, 2018

Conversation

shortdiv
Copy link
Contributor

@shortdiv shortdiv commented Jun 5, 2018

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature

Does this PR introduce a breaking change? (check one)

  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

Other information:

Addresses #6428 and vuejs/v2.vuejs.org#1648

Copy link
Contributor

@chrisvfritz chrisvfritz left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @shortdiv!

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Jul 31, 2018

@yyx990803 Is there anything more that should be done on this? Being able to universally recommend kebab-case event names will simplify a couple sections of the docs and allow us to add this rule to the ESLint plugin. 🙂

syncGen = genAssignmentCode(value, `$event`)
addHandler(
el,
`update:${kebabize(name)}`,
Copy link
Member

Choose a reason for hiding this comment

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

The hyphenate function might be a better fit here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! 🙂 I forgot that one existed. @shortdiv, can you update to use hyphenate instead?

export const kebabize = cached((str: string): string => {
return str.replace(kebabizeRE, (m, p1, p2) => `${p1}-${p2}`).toLowerCase()
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're no longer using this, can you delete it please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got it!

@yyx990803
Copy link
Member

yyx990803 commented Nov 30, 2018

// TODO revert changes in #8845 that will become unnecessary after merging this

@yyx990803 yyx990803 added this to Todo in 2.6 Dec 5, 2018
@Justineo
Copy link
Member

// TODO revert changes in #8845 that will become unnecessary after merging this

Only this line can be reverted. But I think it's okay to keep it as is.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Dec 17, 2018

Thinking about this a little more, I wonder if addHandler itself should actually create both camelCase and kebab-case listeners for every event. This would fix cases where a library is emitting a camelCased event that can't be listened to in HTML (even though we warn against it in the docs) and also solve a JSX issue. Thoughts?

@yyx990803 yyx990803 moved this from Todo to In progress in 2.6 Dec 20, 2018
@yyx990803 yyx990803 changed the base branch from dev to 2.6 December 21, 2018 18:28
@yyx990803
Copy link
Member

@chrisvfritz that would significantly bloat the compiled output, not sure if it's worth it, especially when this is not an issue when using string templates.

@yyx990803 yyx990803 merged commit 3fca527 into vuejs:2.6 Dec 21, 2018
2.6 automation moved this from In progress to Done Dec 21, 2018
yyx990803 added a commit that referenced this pull request Dec 21, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.6
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants