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:#6938 <keep-alive> should not cache anonymous components #6985

Closed
wants to merge 3 commits into from

Conversation

cuteyumiko
Copy link
Contributor

@cuteyumiko cuteyumiko commented Nov 1, 2017

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

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

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

  • Yes
  • No

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

The PR fulfills these requirements:

Other information:
anonymous components aslo have tag , so func getComponentName can not be used anymore.

@cuteyumiko cuteyumiko changed the title fix:#6938 fix:#6938 <keep-alive> should not cache anonymous components Nov 1, 2017
@cuteyumiko
Copy link
Contributor Author

@jkzing unit test added, please review

@@ -85,8 +85,8 @@ export default {
const componentOptions: ?VNodeComponentOptions = vnode && vnode.componentOptions
if (componentOptions) {
// check pattern
const name: ?string = getComponentName(componentOptions)
if (name && (
const name: ?string = componentOptions && componentOptions.Ctor.options.name
Copy link
Contributor

@dsonet dsonet Nov 1, 2017

Choose a reason for hiding this comment

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

why do you change the way of getting name? Plus doesn't need componentOptions && since it's already in the if branch.
IMO it should keep original logic opts.Ctor.options.name || opts.tag even if you want an optimization here which may not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for review , already modified.

docs descripition:
The match is first checked on the component’s own name option, then its local registration name (the key in the parent’s components option) if the name option is not available. Anonymous components cannot be matched against.

Anonymous components always have a registered name( tag in code) , i think the descripition of docs may cause logical hole.

Copy link
Member

@jkzing jkzing Nov 2, 2017

Choose a reason for hiding this comment

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

@cuteyumiko IMO we may need to keep opts.tag. For example:

const foo = {
  template: `<div>foo</div>`
}

new Vue({
  el: '#app',
  components: {
    foo
  }
})

In above case, you'll find Ctor.options doesn't have a name property, but there actually is tag in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ehhh..i see
i have done another test for caching anonymous components , it turns out caching logic is ok

#6938 it's problem may be casued by vue-router , i tracked this issue in a wrong way

@cuteyumiko cuteyumiko closed this Nov 2, 2017
@PanJiaChen
Copy link

@cuteyumiko why close the pr ?

@cuteyumiko
Copy link
Contributor Author

@PanJiaChen it works fine in offical unit test .your issue may be casued by vue-router .

@PanJiaChen
Copy link

PanJiaChen commented Nov 17, 2017

may be is vue-router bug or vue. but the problem is objective existence.
if a router has not set name :

function getComponentName (opts) {
  return opts && (opts.Ctor.options.name || opts.tag)
}

this function will return undefined, so has some problems when use keep-alive
in

const name: ?string = getComponentName(componentOptions)
if (name && (
(this.exclude && matches(this.exclude, name)) ||
(this.include && !matches(this.include, name))
)) {
return vnode
}

detail:#6938
@yyx990803

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