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: keep the overrides prototype information of component #856

Merged
merged 1 commit into from Jul 29, 2018

Conversation

kuitos
Copy link
Contributor

@kuitos kuitos commented Jul 27, 2018

add the component prototype override scenario and remove the version condition.
@eddyerburgh Actually I don't know why we need add a version condition here: cacfda8 , any special reason?

@eddyerburgh eddyerburgh merged commit 0371793 into vuejs:dev Jul 29, 2018
@eddyerburgh
Copy link
Member

This change is actually problematic.

We have an issue with components that already been extended (with Vue.extend). They never extend from the localVue class when they are instantiated, so they don't inherit assets like components set in stubs, and properties set in mocks.

The original code removed by this PR extended all components, including class components, EXCEPT in Vue < 2.3, where components extended with Vue.extend did not merge props correctly.

I would like to revert this PR, because it stops us from being able to add assets to the component without mutating it. But I can see that the previous code was causing you issues in your tests.

Can you give me some details about why your tests don't work when the component is extended?

@kuitos
Copy link
Contributor Author

kuitos commented Jul 31, 2018

@eddyerburgh Sure.
As we had overridden the prototype of extended component in my test, for some hijack requirements, when vue >= 2.3 in original code the process would go to _Vue.extend(component).extend(instanceOptions).

From the implementation of Vue.extend:

Vue.extend = function (extendOptions: Object): Function {
    extendOptions = extendOptions || {}
    const Super = this
    ...
    Sub.prototype = Object.create(Super.prototype)
    ...

We can see that if we call _Vue.extend(component), the Super will represent Vue and Sub.prototype will extend from Vue.prototype rather than component.prototype, then the component prototype which we had overridden would be lost.

I would like to revert this PR, because it stops us from being able to add assets to the component without mutating it.

Shall we add some unit test to explain and guarantee this scenario?

@eddyerburgh
Copy link
Member

Shall we add some unit test to explain and guarantee this scenario?

Yes definitely, I made a mistake by merging this PR because no tests were failing.

I'd also like to make sure your use case works as expected, but I can't see an easy solution.

@kuitos
Copy link
Contributor Author

kuitos commented Jul 31, 2018

uh... Could you show some code to explain what u wanna achieve? Actually I don't really know yet😂

@eddyerburgh
Copy link
Member

eddyerburgh commented Jul 31, 2018

const Constructor = vueVersion < 2.3 && typeof component === 'function'
? component.extend(instanceOptions)
: _Vue.extend(component)

@kuitos
Copy link
Contributor Author

kuitos commented Jul 31, 2018

Could you please provide a test case to describe it?

@eddyerburgh
Copy link
Member

eddyerburgh commented Jul 31, 2018

it('adds variables to extended components', () => {
  const TestComponent = Vue.extend({
    template: `
      <div>
        {{$route.path}}
      </div>
    `
  })
  const $route = { path: 'http://test.com' }
  const wrapper = mountingMethod(TestComponent, {
    mocks: {
      $route
    }
  })
  const HTML =
    mountingMethod.name === 'renderToString' ? wrapper : wrapper.html()
  expect(HTML).contains('http://test.com')
})

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.

None yet

2 participants