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(#8728): only execute the dependArray function once #8734

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

HcySunYang
Copy link
Member

What kind of change does this PR introduce? (check at least one)

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:
The newDepIds property of each Watcher instance object holds all the dependencies collected by this evaluation. We can prevent the dependArray function from being called multiple times by checking if the dependency has been collected.

@Yuyz0112
Copy link

em.. I think the changes in this PR cause a regression bug.

It looks like the dependArray method was designed to solve the dependency problem in a nested array.

<template>
  <div>
    <p>{{ nest[0] }}</p>
    <button @click="nest[0].push('next')">add one</button>
  </div>
</template>

<script>
export default {
  data() {
    return {
      nest: [
        ['init'],
      ],
    };
  },
}
</script>

Have not try this example with your branch yet, but I think the checkRelated will cause the nest array not reactive.

@HcySunYang
Copy link
Member Author

The dependArray function will still execute, except that it will only execute once.
Watcher collects dependencies by executing getters, and each time the getter is executed, the watcher.cleanupDeps function is executed to reset newDepIds. That is, everything is a new start every time the watcher's getter is re-executed, but in the process, there is no need to execute the dependArray function multiple times. It does not cause nested arrays to be not reactive.

@Yuyz0112
Copy link

my bad, the check was executed before depend so it is ok:)

Copy link

@wangyi7099 wangyi7099 left a comment

Choose a reason for hiding this comment

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

How about

const needDepend = !Dep.target.checkRelated(dep)
   if (Dep.target && needDepend ) {
        dep.depend()
        if (childOb) {
          childOb.dep.depend()
          if (Array.isArray(value)) {
        
            dependArray(value)
        }

Because plain-object will still cause useless redepend.

@HcySunYang
Copy link
Member Author

@wangyi7099
When value is a plain-object, your code will cause it to fail to collect dependencies. And performance issues only occur in the case of large arrays, usually we don't need to care about objects

@wangyi7099
Copy link

wangyi7099 commented Sep 14, 2018

@HcySunYang
I'm sorry. My sample code has some errors. I have updated it.

And performance issues only occur in the case of large arrays, usually we don't need to care about objects

I think you are right. But I also think care about plain-object will also save some performance cost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants