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(runtime-core): schedulerJob skip correctly #2834

Closed
wants to merge 2 commits into from

Conversation

edison1105
Copy link
Member

@edison1105 edison1105 commented Dec 16, 2020

close #2768
close #2829
caused by this af95604

after queue.splice(i,1) in invalidateJob,the length of queue will change, resulting in incomplete job execution in flushJobs

@edison1105 edison1105 changed the title fix(runtime-core): flushIndex should -1 when queue splice fix(runtime-core): schedulerJob allow to skip avoid component double update Dec 17, 2020
@Mr14huashao
Copy link

@edison1105,Hi,i find a problem.
The skip attribute is meaningless and becomes undefined during compilation.

if (job && !job.skip)    //The skip is always undefined.

The button still responds properly after the skip is deleted.

export function invalidateJob(job: SchedulerJob) {
  const i = queue.indexOf(job)
  if (i > -1) {
    //queue[i].skip = true
  }
}

Is there a better way to deal with such situations?

@edison1105
Copy link
Member Author

@Mr14huashao
First of all, invalidateJob is to check whether the job of the child update is already in the queue.

If it is already in the queue, it should not be executed repeatedly. It also works if you remove queue[i].skip = true, but the job will be executed multiple times.this is totally unnecessary.

@Mr14huashao
Copy link

@edison1105
After the sourceMap test, I find that job.skip is always undefined.
In other words,! job.skip is always true.

if (job && !job.skip)

The job cannot be determined. It is more reasonable to exclude the problem of the af95604 queue[i] = null.
Maybe we should deal with the queue a different way.

@edison1105 edison1105 changed the title fix(runtime-core): schedulerJob allow to skip avoid component double update fix(runtime-core): schedulerJob allow to skip avoid component double update correctly Jan 11, 2021
@edison1105 edison1105 changed the title fix(runtime-core): schedulerJob allow to skip avoid component double update correctly fix(runtime-core): schedulerJob to skip correctly Jan 11, 2021
@edison1105 edison1105 changed the title fix(runtime-core): schedulerJob to skip correctly fix(runtime-core): schedulerJob skip correctly Jan 11, 2021
@Ryan2128
Copy link

Why is this PR still not merged?

@hgm-gx
Copy link

hgm-gx commented Jan 28, 2021

临时解决方法:

watch:{
    'targetValue':function(val){
      this.refreshTarget = false
      this.$nextTick(function(){
        this.refreshTarget = true
      })
    },
  },

@LinusBorg
Copy link
Member

Hey @edison1105 do you have an idea for a test case?

@LinusBorg LinusBorg added 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. labels Feb 4, 2021
@edison1105
Copy link
Member Author

edison1105 commented Feb 4, 2021

Hey @edison1105 do you have an idea for a test case?

done!

@github-actions
Copy link

github-actions bot commented Feb 5, 2021

Size report

Path Size
vue.global.prod.js 40.56 KB (-0.01% 🔽)
runtime-dom.global.prod.js 26.95 KB (0%)
size-check.global.prod.js 16.31 KB (-0.01% 🔽)

@edison1105 edison1105 force-pushed the fix/2768 branch 2 times, most recently from f512ba6 to b8990d2 Compare February 5, 2021 06:28
fix(runtime-core): schedulerJob allow to skip avoid component double update

fix(runtime-core): schedulerJob allow to skip avoid component double update

fix: improve

test: modify test case
@LinusBorg LinusBorg removed the need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. label Feb 5, 2021
@LinusBorg LinusBorg added this to Planned, might need fresh review in Next Patch Feb 5, 2021
@LinusBorg
Copy link
Member

There's ongoing discussion about another way to solve this here: #3184

We should resolve that discussion before deciding wether to merge this PR or the other one.

@LinusBorg LinusBorg added 🛑 on hold This PR can't be merged until other work is finished need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. labels Feb 7, 2021
@LinusBorg LinusBorg moved this from Planned, might need fresh review to Guidance needed in Next Patch Feb 7, 2021
@yyx990803
Copy link
Member

I believe #3184 provides a more robust fix for this, but thanks a lot of this PR as it provides important information for #3184 to build upon.

@yyx990803 yyx990803 closed this Feb 25, 2021
@yyx990803 yyx990803 removed this from Guidance needed in Next Patch Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. 🛑 on hold This PR can't be merged until other work is finished
Projects
None yet
7 participants