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 provide isn't reactive with a single array, Fix #5223 #5229

Merged
merged 2 commits into from
Mar 21, 2017

Conversation

Kingwl
Copy link
Member

@Kingwl Kingwl commented Mar 20, 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:

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:
#5223

@HerringtonDarkholme
Copy link
Member

Related: #5067

Would @znck please also give a look?

@znck
Copy link
Member

znck commented Mar 20, 2017

One thing I don't want with provide/inject is child modifying ancestor's data.

If foo is injected & reactive then setting this.foo = null would set the provided prop to null. IMO that could be problematic.

Injected properties were kept non-reactive intentionally. Perhaps @yyx990803 could explain better.

@Kingwl
Copy link
Member Author

Kingwl commented Mar 20, 2017

https://facebook.github.io/react/docs/context.html#updating-context

React has an API to update context, but it is fundamentally broken and you should not use it.

should we warn when the context is changed?

@znck
Copy link
Member

znck commented Mar 20, 2017

Currently, reactive objects can be provided.

https://jsfiddle.net/m0fyr6z1/3/ - In this example, you see, child re-renders if array is changed in parent component.

IMO, rather than setting provided injections (current implementation) or defining reactive injections (this PR), it would be better to add enumerable, non-configurable & read-only injections.

@znck
Copy link
Member

znck commented Mar 20, 2017

/ping @yyx990803 – Your input required.

@yyx990803
Copy link
Member

defineReactive accepts a fourth argument (customSetter) which allows overriding the default set behavior. We can just use that to do the warning when a child component attempts to mutate an injection binding. This is what we are doing for props too: https://github.com/vuejs/vue/blob/dev/src/core/instance/state.js#L71-L91

@@ -29,7 +31,14 @@ export function initInjections (vm: Component) {
let source = vm
while (source) {
if (source._provided && provideKey in source._provided) {
vm[key] = source._provided[provideKey]
defineReactive(vm, key, source._provided[provideKey], () => {
warn(
Copy link
Member

Choose a reason for hiding this comment

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

make sure to prefix warnings with process.env.NODE_ENV check :)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@yyx990803 yyx990803 merged commit 7dea9f1 into vuejs:dev Mar 21, 2017
@Kingwl Kingwl changed the title fix provide isn't reactive with a single array fix provide isn't reactive with a single array, Fix #5223 Mar 21, 2017
@Kingwl Kingwl deleted the dev-reactive-array branch March 21, 2017 09:13
@rayrutjes
Copy link
Contributor

Overriding the setter makes this PR introduce a BC.

@yyx990803
Copy link
Member

It doesn't because it was not reactive anyway.

@rayrutjes
Copy link
Contributor

In the project I'm working on, I did the following:

export default {
  inject: ['_searchStore'],
  props: {
    searchStore: {
      type: Object,
      default () {
        return this._searchStore
      }
    }
  }
}

Which will now break if I'm not mistaken.

awamwang pushed a commit to awamwang/vue that referenced this pull request Jun 15, 2017
* fix provide isn't reactive with a single array - Fix vuejs#5223

* add warning when injections has been modified
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

5 participants