Skip to content

Conversation

Picknight
Copy link
Contributor

Even if normal effects that depends on computed effects is executed first, After the execution of computed effects will also trigger normal effects again.
So it seems we don't need computedRunners

@yyx990803
Copy link
Member

That would mean the normal effect has to run twice - which is exactly what we are trying to avoid.

@yyx990803 yyx990803 closed this Jun 29, 2020
@Picknight
Copy link
Contributor Author

Picknight commented Jul 1, 2020

I simulated a special case of normal effects depend on computed effects, not sure if it is correct.

const reactVal = reactive({})
let computedVal = computed(() => reactVal.foo)
let effectVal
effect(() => {
  effectVal = reactVal.foo + computedVal.value
  console.log('normal effects trigger!')
})

console.log('--- Test begin ---')
reactVal.foo = 1

Regardless of whether computedRunners are used, normal effects trigger! will be printed twice after the last line is executed.

The reason is that trigger(computed, 'set', 'value') is executed in the scheduler function of computed.ts.

No matter who executes normal effects and computed effects first, normal effects will be executed once after the computed effects are executed.

So, I didn’t see the necessity of using computedRunners. @yyx990803

@yyx990803
Copy link
Member

Yeah you are right.

This was necessary in the case of using an async scheduler (exposed as watchEffect), before we added effect id-based sorting in the scheduler itself. But now the scheduler has id-based sorting so the effects are always flushed based on the order of creation, making this no longer necessary.

@yyx990803 yyx990803 merged commit 5c490f1 into vuejs:master Jul 1, 2020
@Picknight Picknight deleted the computed branch July 2, 2020 02:25
@Picknight
Copy link
Contributor Author

Thanks for your explanation let me understand the historical reason here.

@jods4
Copy link
Contributor

jods4 commented Jul 15, 2020

There might be some clean up left to do.

computed is still exposed in ReactiveEffectOptions, although it's not used anymore:
https://github.com/vuejs/vue-next/blob/master/packages/reactivity/src/effect.ts#L24

And it's set by apiWatch, to no effect:
https://github.com/vuejs/vue-next/blob/58b07069ad33c8a8e44cb47b81084a452dda2846/packages/runtime-core/src/apiWatch.ts#L278-L279

@Picknight
Copy link
Contributor Author

Has been cleaned up in [chore: remove outdated options].

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.

3 participants