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

Merge inject when extending a component #5827

Merged
merged 9 commits into from
Jun 15, 2017
Merged

Conversation

jkzing
Copy link
Member

@jkzing jkzing commented Jun 5, 2017

#5603

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:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@jkzing
Copy link
Member Author

jkzing commented Jun 5, 2017

Actually I have been seeing this issue for several days. But I'm not sure that if the behavior inject is not extending is expected or not, because from code level it seems this use case was not taken into consideration before.

So please let me know if this use case valid, and I'll add test case to it.

*/
function normalizeInject (options: Object) {
const inject = options.inject
if (!inject) return
Copy link
Member

Choose a reason for hiding this comment

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

The normalization is only needed if inject is an Array, so we can just return if it is not an Array.

Copy link
Member

Choose a reason for hiding this comment

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

In addition, with normalization here, we should be able to simplify the resolveInject function here since the normalized value will always be objects: https://github.com/vuejs/vue/blob/dev/src/core/instance/inject.js#L38

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, will look into it later.

@yyx990803 yyx990803 merged commit 080c387 into vuejs:dev Jun 15, 2017
@jkzing
Copy link
Member Author

jkzing commented Jun 15, 2017

@yyx990803 thanks 🎉

@jkzing jkzing deleted the extend-inject branch June 15, 2017 14:20
awamwang added a commit to awamwang/vue that referenced this pull request Jun 28, 2017
* 'dev' of https://github.com/vuejs/vue:
  build: use cross-platform hook installation with shelljs
  build: move test config files into /test
  build: add script for generating release note
  build: add git commit message validation
  fix(v-model): use consistent behavior during IME composition for other text-like input types (fix vuejs#5902)
  simplify source with rest params
  fix slot resolved incorrect with abstract component (fix vuejs#5888) (vuejs#5895)
  test:improve reserved props test
  test:add bind object test
  fix:when using object syntax in v-bind, special attribute have no effect
  Be able to use string type index in array (vuejs#5889)
  Merge inject when extending a component (vuejs#5827)

# Conflicts:
#	.gitignore
#	src/core/util/options.js
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.

2 participants