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: no unnecessary extend #1084

Closed
wants to merge 3 commits into from

Conversation

eddyerburgh
Copy link
Member

Currently create a subclass of a component with extend two times, in order to apply instance options. This causes some component options to not have the same own props as they would in production (e.g. #1067).

One solution is to stop calling extend twice. The solution in this PR means that you can no longer override component options like methods, or computed, using instance options.

The instance options would instead be used to instantiate the root Vue instance, so store, route, can still be passed.

This is a large breaking change:

  • Remove methods option
  • Removes ability to overwrite methods, computed, or other component properties with instance options

This PR is a proof of concept as I investigate alternative approaches to fixing #1067.

Copy link

@grusingh grusingh left a comment

Choose a reason for hiding this comment

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

Removing calls to extend twice seems good. :)
Review summary: There seems a typo in documentation and one of code example could be improved.

@@ -292,29 +292,21 @@ When `sync` is `false`, the Vue component is rendered asynchronously.

## Other options

When the options for `mount` and `shallowMount` contain the options other than the mounting options, the component options are overwritten with those using [extends](https://vuejs.org/v2/api/#extends).
When the options for `mount` and `shallowMount` contain the options other than the mounting options, the options are added used when the Vue instance is initialized.
Copy link

Choose a reason for hiding this comment

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

When the options for mount and shallowMount contain the options other than the mounting options, the options are added used when the Vue instance is initialized.

return 'b'
}
}
template: `
Copy link

Choose a reason for hiding this comment

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

This example could be improved to make context more clear.
expect(wrapper.text()).toBe('aBC') would fail?

@@ -90,8 +90,8 @@ export default function createInstance (
// extend component from _Vue to add properties and mixins
// extend does not work correctly for sub class components in Vue < 2.2
const Constructor = typeof component === 'function'
? _Vue.extend(component.options).extend(instanceOptions)
: _Vue.extend(component).extend(instanceOptions)
? _Vue.extend(component.options)
Copy link

Choose a reason for hiding this comment

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

👍

@pipopotamasu pipopotamasu mentioned this pull request Jun 28, 2019
13 tasks
@JessicaSachs
Copy link
Collaborator

We're closing this experimental PR for now. The issue is still outstanding, but the API change and approach would break too many tests for our users.

We will revisit approaches to solving the initial issue as time goes on.

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.

3 participants