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(reactivity): re-fix #10114 #10123

Merged
merged 6 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 32 additions & 6 deletions packages/reactivity/__tests__/computed.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -454,14 +454,10 @@ describe('reactivity/computed', () => {
expect(fnSpy).toBeCalledTimes(2)
})

it('...', () => {
const fnSpy = vi.fn()
it('should chained recurse effects clear dirty after trigger', () => {
const v = ref(1)
const c1 = computed(() => v.value)
const c2 = computed(() => {
fnSpy()
return c1.value
})
const c2 = computed(() => c1.value)

c1.effect.allowRecurse = true
c2.effect.allowRecurse = true
Expand All @@ -471,6 +467,36 @@ describe('reactivity/computed', () => {
expect(c2.effect._dirtyLevel).toBe(DirtyLevels.NotDirty)
})

it('should chained computeds dirtyLevel update with first computed effect', () => {
const v = ref(0)
const c1 = computed(() => {
if (v.value === 0) {
v.value = 1
}
return v.value
})
const c2 = computed(() => c1.value)
const c3 = computed(() => c2.value)

c3.value

expect(c1.effect._dirtyLevel).toBe(DirtyLevels.Dirty)
expect(c2.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty)
expect(c3.effect._dirtyLevel).toBe(DirtyLevels.MaybeDirty)
})

it('should work when chained(ref+computed)', () => {
const value = ref(0)
const consumer = computed(() => {
value.value++
return 'foo'
})
const provider = computed(() => value.value + consumer.value)
expect(provider.value).toBe('0foo')
expect(provider.effect._dirtyLevel).toBe(DirtyLevels.Dirty)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it is good practice to leave a computed dirtyLevel as Dirty after accessing its value.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is correct and reasonable, because provider deps are indeed changed during accessing its value.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's OK, but then we should find a way to triggerEffects even computed are dirty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it works with the latest commit.

expect(provider.value).toBe('1foo')
})

it('should be not dirty after deps mutate (mutate deps in computed)', async () => {
const state = reactive<any>({})
const consumer = computed(() => {
Expand Down
3 changes: 3 additions & 0 deletions packages/reactivity/src/computed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,9 @@ export class ComputedRefImpl<T> {
}
}
trackRefValue(self)
if (self.effect._dirtyLevel >= DirtyLevels.MaybeDirty) {
triggerRefValue(self, DirtyLevels.MaybeDirty)
}
return self._value
}

Expand Down
5 changes: 1 addition & 4 deletions packages/reactivity/src/effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -295,10 +295,7 @@ export function triggerEffects(
// when recurse effect is running, dep map could have outdated items
continue
}
if (
effect._dirtyLevel < dirtyLevel &&
!(effect._runnings && !effect.allowRecurse)
) {
if (effect._dirtyLevel < dirtyLevel) {
const lastDirtyLevel = effect._dirtyLevel
effect._dirtyLevel = dirtyLevel
if (lastDirtyLevel === DirtyLevels.NotDirty) {
Expand Down