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

fix(runtime-core): fix inconsistent event casing for render functions, see issue#2249 #2278

Merged
merged 2 commits into from
Oct 6, 2020

Conversation

shadowings-zy
Copy link
Contributor

@shadowings-zy shadowings-zy commented Sep 30, 2020

In issue #2249 , we found inconsistent event casing for render functions.

The reproduction link in issue#2249 is here:
https://codesandbox.io/s/nervous-surf-f5lbs

In that case, when we emit test-event, only onTest-Event worked. But the one expected to work on the render function should be onTestedEvent.

So I fix it in this PR, and add test case for it.

All test cases were passed.

Close #2249

@LinusBorg LinusBorg added the need test The PR has missing test cases. label Sep 30, 2020
@shadowings-zy shadowings-zy force-pushed the fix#2249 branch 2 times, most recently from ef0d319 to 55c7686 Compare October 1, 2020 04:12
@shadowings-zy
Copy link
Contributor Author

@LinusBorg Hello, I updated my test case. I think the new test case will meet our expectations.

: )

@posva
Copy link
Member

posva commented Oct 1, 2020

Can you add dev warning like in vue 2 (#2249 (comment))

@LinusBorg
Copy link
Member

I think this will break in-browser templates:

<the-component v-on:test-event="handler">

results in this render function:

return (_openBlock(), _createBlock(_component_Mycomponent, { "onTest-event": _ctx.handler }, null, 8 /* PROPS */, ["onTest-event"]))

Example in template explorer

So I think the camelCasing needs to be done in the compiler as well.

@shadowings-zy
Copy link
Contributor Author

I think this will break in-browser templates:

<the-component v-on:test-event="handler">

results in this render function:

return (_openBlock(), _createBlock(_component_Mycomponent, { "onTest-event": _ctx.handler }, null, 8 /* PROPS */, ["onTest-event"]))

Example in template explorer

So I think the camelCasing needs to be done in the compiler as well.

@LinusBorg Yes, when I started fixing this bug, I had already considered fix event name in in-browser templates.

But when I read the setFullProps function in packages/runtime-core/src/componentProps.ts file

function setFullProps(
  instance: ComponentInternalInstance,
  rawProps: Data | null,
  props: Data,
  attrs: Data
) {
  const [options, needCastKeys] = instance.propsOptions
  if (rawProps) {
    for (const key in rawProps) {
      const value = rawProps[key]
      // key, ref are reserved and never passed down
      if (isReservedProp(key)) {
        continue
      }
      // prop option names are camelized during normalization, so to support
      // kebab -> camel conversion here we need to camelize the key.
      let camelKey
      if (options && hasOwn(options, (camelKey = camelize(key)))) {
        props[camelKey] = value
      } else if (!isEmitListener(instance.emitsOptions, key)) {
        // Any non-declared (either as a prop or an emitted event) props are put
        // into a separate `attrs` object for spreading. Make sure to preserve
        // original key casing
        attrs[key] = value
      }
    }
  }

  if (needCastKeys) {
    const rawCurrentProps = toRaw(props)
    for (let i = 0; i < needCastKeys.length; i++) {
      const key = needCastKeys[i]
      props[key] = resolvePropValue(
        options!,
        rawCurrentProps,
        key,
        rawCurrentProps[key],
        instance
      )
    }
  }
}

I see a comment "Any non-declared (either as a prop or an emitted event) props are put into a separate attrs object for spreading. Make sure to preserve original key casing."

So I think maybe in in-browser templates we need to make sure to preserve original key casing? I am not sure.

If not, I will work on fixing camelCasing in compiler.

: )

@shadowings-zy
Copy link
Contributor Author

Can you add dev warning like in vue 2 (#2249 (comment))

@posva Ok, I can add dev warning for in-DOM templates. But I want to make sure that is the warning context is the same as Vue2?

The warning you mentioned in issue #2278 is :

You are running Vue in development mode.
Make sure to turn on production mode when deploying for production.
See more tips at https://vuejs.org/guide/deployment.html

@posva
Copy link
Member

posva commented Oct 1, 2020

@shadowings-zy That's not the warning, if you run the reproduction on the comment you will see it

@shadowings-zy
Copy link
Contributor Author

@shadowings-zy That's not the warning, if you run the reproduction on the comment you will see it

@posva Hello, the reproduction on the comment has nothing but an empty Vue2 project......
截屏2020-10-02 12 16 58

So I code an in-Dom template myself by using Vue2. And I found two warnings:

One is in extract props:

            `Prop "${keyInLowerCase}" is passed to component ` +
            `${formatComponentName(tag || Ctor)}, but the declared prop name is` +
            ` "${key}". ` +
            `Note that HTML attributes are case-insensitive and camelCased ` +
            `props need to use their kebab-case equivalents when using in-DOM ` +
            `templates. You should probably use "${altKey}" instead of "${key}".`

Another is in emit event:

          `Event "${lowerCaseEvent}" is emitted in component ` +
          `${formatComponentName(vm)} but the handler is registered for "${event}". ` +
          `Note that HTML attributes are case-insensitive and you cannot use ` +
          `v-on to listen to camelCase events when using in-DOM templates. ` +
          `You should probably use "${hyphenate(event)}" instead of "${event}".`

I think both of them need to added in Vue3.

If so, I will add them. If not, which one is need to add for Vue3?

: )

@posva
Copy link
Member

posva commented Oct 2, 2020

Yeah, it's the second one (https://github.com/vuejs/vue/blob/dev/src/core/instance/events.js#L123). Weird the jsfiddle wasn't what I had, probably forgot to press fork 😓

@shadowings-zy
Copy link
Contributor Author

@posva @LinusBorg
Hello, I have added warnings for in-DOM template, and doing camelCasing in the compiler in the newest PR.
All tests were passed.
Let us review it.

@yyx990803 yyx990803 merged commit 62f2617 into vuejs:master Oct 6, 2020
@yaquawa
Copy link

yaquawa commented Oct 19, 2020

Hi there, I found my code suddenly didn't work after this merge :

<foo @my-event-name="callEvent" />
this.$emit('my-event-name') // `callEvent` not triggered

would you please fix this issue ?

@LinusBorg
Copy link
Member

Please open a bug. Commments on merged commits get lost quickly.

@yaquawa
Copy link

yaquawa commented Oct 19, 2020

@LinusBorg done #2429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need test The PR has missing test cases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent event casing for render functions
5 participants