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(mergeData): skip recursive call if values are identical #8967

Merged
merged 1 commit into from Nov 30, 2018

Conversation

Projects
None yet
5 participants
@KaelWD
Copy link
Contributor

commented Oct 18, 2018

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:

Other information:
Fixes vuetifyjs/vuetify#5633

I've been unable to reproduce this in isolation, but have run into it several times in the past. This check should also reduce unnecessary calls to mergeData, I noticed thousands of instances of identical objects being merged that didn't cause errors.

@posva

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

I don't know what scenario causes this, but shouldn't be possible to reproduce with

const a = { a: 'a' }
const b = { a }
const c = { a }
// making the code call mergeData(b, c) somewhere
@KaelWD

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

Yeah, but I don't know where to put b and c to make that happen. It only causes a stack overflow if a has recursive references, in our case it's a provided vue instance. In your example it would just be an unnecessary loop over a that doesn't do anything as all the properties obviously exist in both arguments. You can see some more information in the linked issue https://github.com/vuetifyjs/vuetifyjs.com/issues/630

@posva

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

It's worth checking what the actual source of the problem is. There could be another problem somewhere else. That being said, the change is a good idea IMO

@KaelWD

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

There's a similar issue #6190, but that's trying to merge two different vue objects so I don't think this patch will fix it unless it's getting hung up on $root or something.

John thinks this one has something do do with component reuse, because it only happens when you leave the page for another one. But yeah this does fix our particular problem, and probably has a small performance boost too so there's no reason not to include it.

@jvbianchi

This comment has been minimized.

Copy link

commented Nov 9, 2018

Any plans on merging this?

@jrvaja

This comment has been minimized.

Copy link

commented Nov 11, 2018

@KaelWD Hope this will be merged in the up coming sprint.

@yyx990803 yyx990803 merged commit a7658e0 into vuejs:dev Nov 30, 2018

5 checks passed

ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint-flow-types Your tests passed on CircleCI!
Details
ci/circleci: test-cover Your tests passed on CircleCI!
Details
ci/circleci: test-e2e Your tests passed on CircleCI!
Details
ci/circleci: test-ssr-weex Your tests passed on CircleCI!
Details

@KaelWD KaelWD deleted the KaelWD:fix/mergeData-recursion branch Dec 1, 2018

f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.