Skip to content

Commit

Permalink
perf(reactivity): ref should not trigger if value did not change
Browse files Browse the repository at this point in the history 
Note: shallowRef will always trigger on assignment because it does not
account for deep mutations

close #1012
  • Loading branch information
@yyx990803
yyx990803 committed Apr 22, 2020
1 parent 7d858a9 commit b0d4df9
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 15 deletions.
24 changes: 24 additions & 0 deletions packages/reactivity/__tests__/ref.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,19 @@ describe('reactivity/ref', () => {
it('should be reactive', () => {
const a = ref(1)
let dummy
let calls = 0
effect(() => {
calls++
dummy = a.value
})
expect(calls).toBe(1)
expect(dummy).toBe(1)
a.value = 2
expect(calls).toBe(2)
expect(dummy).toBe(2)
// same value should not trigger
a.value = 2
expect(calls).toBe(2)
expect(dummy).toBe(2)
})

Expand Down Expand Up @@ -174,6 +182,22 @@ describe('reactivity/ref', () => {
expect(dummy).toBe(2)
})

test('shallowRef force trigger', () => {
const sref = shallowRef({ a: 1 })
let dummy
effect(() => {
dummy = sref.value.a
})
expect(dummy).toBe(1)

sref.value.a = 2
expect(dummy).toBe(1) // should not trigger yet

// force trigger
sref.value = sref.value

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Apr 22, 2020

Contributor

@yyx990803 This is horribly wrong.
shallowRef should behave like ref, only shallow.

This is unintuitive, inconsistent and counter-performant.
Imagine someone depending on this trigger, then "upgrading" its shallowRef to a full ref and not getting a trigger anymore??

If you want to observe mutations of your object, make your object reactive (either directly, or through a deep ref).
If you use a shallowRef, you explicitely say that you don't care about inner mutations.
If you want to use a signal pattern... then create and use an actual signal. With createRef it's actually easy now.

If we need an API to explicitely trigger a ref that hasn't changed (either deep or shallow), then let's add one. Maybe on the ref: sref.trigger() or as I know you don't like that, as a utility function: updateRef(ref).

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Apr 22, 2020

Contributor

Also for consistency with reactive I just checked and shallowReactive behaves like reactive (i.e. it would not trigger in this case).

This comment has been minimized.

Sign in to view

Copy link
@yyx990803

yyx990803 Apr 22, 2020 •
edited
Loading

Author Member

See 2acf3e8

You are right, but please avoid using words like "horribly wrong" because it ruined my mood for a good day.

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Apr 23, 2020

Contributor

@yyx990803 Hey sorry, that was very poor wording. It's not that big a deal.
Please have a nice day, here's a kitten: 🐈
expect(dummy).toBe(2)
})

test('isRef', () => {
expect(isRef(ref(1))).toBe(true)
expect(isRef(computed(() => 1))).toBe(true)
Expand Down
31 changes: 16 additions & 15 deletions packages/reactivity/src/ref.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { track, trigger } from './effect'
import { TrackOpTypes, TriggerOpTypes } from './operations'
import { isObject } from '@vue/shared'
import { reactive, isProxy } from './reactive'
import { isObject, hasChanged } from '@vue/shared'
import { reactive, isProxy, toRaw } from './reactive'
import { ComputedRef } from './computed'
import { CollectionTypes } from './collectionHandlers'

Expand Down Expand Up @@ -43,27 +43,28 @@ export function shallowRef(value?: unknown) {
return createRef(value, true)
}

function createRef(value: unknown, shallow = false) {
if (isRef(value)) {
return value
}
if (!shallow) {
value = convert(value)
function createRef(rawValue: unknown, shallow = false) {
if (isRef(rawValue)) {
return rawValue
}
let value = shallow ? rawValue : convert(rawValue)
const r = {
_isRef: true,
get value() {
track(r, TrackOpTypes.GET, 'value')
return value
},
set value(newVal) {
value = shallow ? newVal : convert(newVal)
trigger(
r,
TriggerOpTypes.SET,
'value',
__DEV__ ? { newValue: newVal } : void 0
)
if (shallow || hasChanged(toRaw(newVal), rawValue)) {

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Apr 22, 2020

Contributor

@yyx990803
Just sayin': other frameworks let you pass your own comparator as an option to the ref.
JS is a bit of a mess as you can't override equality. As you know the most common offender is Date.
Accounting for dates is not 100% though, it's just the most common occurance. E.g. I use Luxon for my dates, which suffer the same problem.
Doing a valueOf comparison goes some way as most objects don't implement it (i.e. return themselves) and stuff like Date or Luxon DateTime would return their timestamp.
It's not 100% and might have false positives (depending on how valueOf is implemented); and other kind of "value" types can't easily produce unique valueOf (e.g. complex numbers { r: 1, i: 2 }).

This comment has been minimized.

Sign in to view

Copy link
@aztalbot

aztalbot Apr 22, 2020

Contributor

pass your own comparator as an option to the ref

@jods4 with customRef, can't you make your own comparableRef(value, comparator) function to account for advanced or edge cases?

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Apr 23, 2020

Contributor

@aztalbot absolutely, at least for refs you can customize this with relative ease thanks to customRef.
The behavior of reactive is not officially tweakable.

This means "false" triggers when a value changes (in a=== sense) but is actually the same, Date being the prime (but not only) example. It's not the end of the world either.

I was wondering if it would be acceptable to make hasChanged a configurable global function in @vue/reactivity. That way we don't incur the cost of including it in every ref and reactive but it can still be changed to account for types used in app.

It's highly unlikely that you need a per-reactive behavior on that regard. Only caveat would be: can it have a negative impact on libraries? Probably yes if you do silly things but then wouldn't it be your fault?

I'm not sure what's the best solution for this.
rawValue = newVal
value = shallow ? newVal : convert(newVal)
trigger(
r,
TriggerOpTypes.SET,
'value',
__DEV__ ? { newValue: newVal } : void 0
)
}
}
}
return r
Expand Down

3 comments on commit b0d4df9

@mika76
Copy link

@mika76 mika76 commented on b0d4df9 Apr 23, 2020

Choose a reason for hiding this comment

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

This is interesting - does this mean it will do a deep compare? Also is this valid for computed's as well?

@jods4
Copy link
Contributor

@jods4 jods4 commented on b0d4df9 Apr 23, 2020

Choose a reason for hiding this comment

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

@mika76 no, ref or shallowRef trigger change when the value they contain changes, they don't do a deep compare.

That being said, ref is also applying deep reactivity, by wrapping the value it returns with reactive.
What happens next depend on what you put inside the ref but if it's a plain object it will be made reactive too, thus achieving more or less the same results.

Computed works a bit differently. It's a lazy reactive cache for a computation.
When the dependencies of the computation change, computed will trigger to notify its value has changed, but it won't recompute its value until you actually read it again.
This implies that it can't compare if the computed value has changed when triggering, not even shallowly.

@mika76
Copy link

@mika76 mika76 commented on b0d4df9 Apr 23, 2020

Choose a reason for hiding this comment

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

@jods4 ah ok thanks for the explanation

Please sign in to comment.