Added check in merge strat for watches if child is already an array (fix #5652) #5653
Conversation
If the child is already an array it does not need to be wrapped again. Fixing #5652
strantr
changed the title
|
Could you add a test for this, please? |
|
Sorry, I checked the PR before the issue, but I'm not sure there's a problem here, mixins are pain javascript objects, not Vue sub classes |
|
@posva It's very reasonable to add this check, to avoid wrap the child value accidentally. |
|
@dsonet Yes, but we have to make sure the accident is possible 😄 |
|
@posva I've added the unit test based on the existence of Object.prototype.watch |
| expect(spy).toHaveBeenCalledWith(2, 1) | ||
| }).then(done) | ||
| }) | ||
| afterEach(function () { |
There was a problem hiding this comment.
I think we could group this test into a describe and use afterAll so the check is run only once. You can also setup the watch property on the beforeAll
There was a problem hiding this comment.
Made the suggested change
|
@posva Any idea why the CI test would fail on "Transition Basic: explicit transition type" - works fine locally and has no relation to my change |
| @@ -172,7 +172,7 @@ strats.watch = function (parentVal: ?Object, childVal: ?Object): ?Object { | |||
| } | |||
| ret[key] = parent | |||
| ? parent.concat(child) | |||
| : [child] | |||
| : Array.isArray(child) ? child : [child] | |||
There was a problem hiding this comment.
So I finally took a proper look at this issue - since the error is caused by native watch on Object.prototype (which is a function), I think the proper fix would be adding the following at the top of the function:
if (typeof parentVal === 'function') {
parentVal = null
}
if (typeof childVal === 'function') {
childVal = null
}Essentially, ignore if the value is Object.prototype.watch. The test case will need to be updated accordingly.
There was a problem hiding this comment.
@yyx990803 I've added a new test for merging multiple layers of extends (I wasn't sure if it was more appropriate in the watch tests or the extends tests) - this causes the issue where child is wrapped multiple times. I am happy to make your change however this second test case will fail with your recommendation but work with the initial fix.
There was a problem hiding this comment.
IMO the initial fix was correct.The core reason caused bug was because it deal with the child value inconsistently. With the fix it can deal with it correctly.
When performing multiple extends/mixins watches are converted to arrays of handlers.
If the parent does not watch an element, but the children do, it will not run the parent.concat code and instead always wrap the child.
If the child is already an array it does not need to be wrapped again.
Fixing #5652
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
devbranch for v2.x (or to a previous version branch), not themasterbranchfix #xxx[,#xxx], where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information: