Skip to content

Commit

Permalink
refactor(reactivity): readonly collections should not track
Browse files Browse the repository at this point in the history
  • Loading branch information
@yyx990803
yyx990803 committed Aug 6, 2020
1 parent ed43810 commit 50adc01
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 23 deletions.
36 changes: 36 additions & 0 deletions packages/reactivity/__tests__/readonly.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,42 @@ describe('reactivity/readonly', () => {
expect(dummy).toBe(2)
})

test('readonly collection should not track', () => {
const map = new Map()
map.set('foo', 1)

const reMap = reactive(map)
const roMap = readonly(map)

let dummy
effect(() => {
dummy = roMap.get('foo')
})
expect(dummy).toBe(1)
reMap.set('foo', 2)
expect(roMap.get('foo')).toBe(2)
// should not trigger
expect(dummy).toBe(1)
})

test('readonly should track and trigger if wrapping reactive original (collection)', () => {
const a = reactive(new Map())
const b = readonly(a)
// should return true since it's wrapping a reactive source
expect(isReactive(b)).toBe(true)

a.set('foo', 1)

let dummy
effect(() => {
dummy = b.get('foo')
})
expect(dummy).toBe(1)
a.set('foo', 2)
expect(b.get('foo')).toBe(2)
expect(dummy).toBe(2)
})

test('wrapping already wrapped value should return same Proxy', () => {
const original = { foo: 1 }
const wrapped = readonly(original)
Expand Down
50 changes: 27 additions & 23 deletions packages/reactivity/src/collectionHandlers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,40 +30,42 @@ const getProto = <T extends CollectionTypes>(v: T): any =>
function get(
target: MapTypes,
key: unknown,
wrap: typeof toReactive | typeof toReadonly | typeof toShallow
isReadonly = false,
isShallow = false
) {
// #1772: readonly(reactive(Map)) should return readonly + reactive version
// of the value
target = (target as any)[ReactiveFlags.RAW]
const rawTarget = toRaw(target)

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Aug 8, 2020

Contributor

You're doing a double toRaw here, because it might be a readonly on top of reactive on top of Map.
That fixes get, but isn't toRaw globally flawed now (because it only undoes one level)?

For instance, the const rawKey = toRaw(key) isn't gonna find the key if I do the following, is that expected/intended?

let key = reactive(new Map)
let readonlyKey = readonly(key)
let m = reactive(new Map)
m.add(key, 1)  // Note: add uses toRaw as key
m.get(readonlyKey) // === undefined, intended?
const rawKey = toRaw(key)
if (key !== rawKey) {
track(rawTarget, TrackOpTypes.GET, key)
!isReadonly && track(rawTarget, TrackOpTypes.GET, key)
}
track(rawTarget, TrackOpTypes.GET, rawKey)
!isReadonly && track(rawTarget, TrackOpTypes.GET, rawKey)
const { has } = getProto(rawTarget)

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Aug 8, 2020 •
edited
Loading

Contributor

Just being curious here: what's the motivation for grabbing the prototype instead of doing rawTarget.has(key), which would be shorter?

As far as I can tell, the only impact is if I'm hacking my Map with custom logic like so:

let myMap = new Map()
myMap.has = function (key) {
  console.log("Do some funny business")
  return Map.prototype.has.call(myMap, key)
}

In which case, quite frankly, I would find surprising that my code isn't called when my Map is wrapped into a reactive?

This comment has been minimized.

Sign in to view

Copy link
@Picknight

Picknight Aug 26, 2020

Contributor

I have the same question.
const wrap = isReadonly ? toReadonly : isShallow ? toShallow : toReactive
if (has.call(rawTarget, key)) {

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Aug 8, 2020

Contributor

You should check the rawKey first, as it's most likely to succeed.

  • For non-reactive keys it makes no difference.
  • Reactive keys added on the reactive collection would have been toRawed by the add handler.
  • So only edge case where a reactive key is added directly to the underlying raw collection remains for the .get(key) case.

BTW is that last case something really worth supporting?
Mixing proxies and non-proxies of the same object is really asking for trouble. You might easily end with 2 different keys for the same object in the collection. So if it's likely to lead to broken code anyway, we may as well simplify Vue.
return wrap(target.get(key))
} else if (has.call(rawTarget, rawKey)) {

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Aug 8, 2020

Contributor

Given that in all cases every wrap is a passthrough for undefined; and that get on missing collection keys returns undefined, I think you could drop that if (has) for slightly shorter and more efficient code.
return wrap(target.get(rawKey))
}
}

function has(this: CollectionTypes, key: unknown): boolean {
const target = toRaw(this)
function has(this: CollectionTypes, key: unknown, isReadonly = false): boolean {
const target = (this as any)[ReactiveFlags.RAW]
const rawTarget = toRaw(target)
const rawKey = toRaw(key)
if (key !== rawKey) {
track(target, TrackOpTypes.HAS, key)
!isReadonly && track(rawTarget, TrackOpTypes.HAS, key)
}
track(target, TrackOpTypes.HAS, rawKey)
const has = getProto(target).has
return has.call(target, key) || has.call(target, rawKey)
!isReadonly && track(rawTarget, TrackOpTypes.HAS, rawKey)
return target.has(key) || target.has(rawKey)
}

function size(target: IterableCollections) {
target = toRaw(target)
track(target, TrackOpTypes.ITERATE, ITERATE_KEY)
return Reflect.get(getProto(target), 'size', target)
function size(target: IterableCollections, isReadonly = false) {
target = (target as any)[ReactiveFlags.RAW]
!isReadonly && track(toRaw(target), TrackOpTypes.ITERATE, ITERATE_KEY)
return Reflect.get(target, 'size', target)
}

function add(this: SetTypes, value: unknown) {

This comment has been minimized.

Sign in to view

Copy link
@jods4

jods4 Aug 8, 2020 •
edited
Loading

Contributor

The return value of add is the Set itself, so that calls can be chained:

var set = new Set().add("x").add("y");

Given that the proxy returns target.add(value), I think the proxification is broken and the non-reactive target is exposed to calling code.

A better implementation should return this, or if you want to support custom maps and sets with non-standard behaviors, check if the implementation is conformant and swap the value if it is:

return result === target ? this : result

The same is true for Map::set, of course.
Expand Down Expand Up @@ -137,15 +139,15 @@ function clear(this: IterableCollections) {
return result
}

function createForEach(isReadonly: boolean, shallow: boolean) {
function createForEach(isReadonly: boolean, isShallow: boolean) {
return function forEach(
this: IterableCollections,
callback: Function,
thisArg?: unknown
) {
const observed = this
const target = toRaw(observed)
const wrap = isReadonly ? toReadonly : shallow ? toShallow : toReactive
const wrap = isReadonly ? toReadonly : isShallow ? toShallow : toReactive
!isReadonly && track(target, TrackOpTypes.ITERATE, ITERATE_KEY)
// important: create sure the callback is
// 1. invoked with the reactive map as `this` and 3rd arg
Expand Down Expand Up @@ -173,19 +175,19 @@ interface IterationResult {
function createIterableMethod(
method: string | symbol,
isReadonly: boolean,
shallow: boolean
isShallow: boolean
) {
return function(
this: IterableCollections,
...args: unknown[]
): Iterable & Iterator {
const target = (this as any)[ReactiveFlags.RAW]
const rawTarget = toRaw(this)
const rawTarget = toRaw(target)
const isMap = rawTarget instanceof Map
const isPair = method === 'entries' || (method === Symbol.iterator && isMap)
const isKeyOnly = method === 'keys' && isMap
const innerIterator = target[method](...args)
const wrap = isReadonly ? toReadonly : shallow ? toShallow : toReactive
const wrap = isReadonly ? toReadonly : isShallow ? toShallow : toReactive
!isReadonly &&
track(
rawTarget,
Expand Down Expand Up @@ -228,7 +230,7 @@ function createReadonlyMethod(type: TriggerOpTypes): Function {

const mutableInstrumentations: Record<string, Function> = {
get(this: MapTypes, key: unknown) {
return get(this, key, toReactive)
return get(this, key)
},
get size() {
return size((this as unknown) as IterableCollections)
Expand All @@ -243,7 +245,7 @@ const mutableInstrumentations: Record<string, Function> = {

const shallowInstrumentations: Record<string, Function> = {
get(this: MapTypes, key: unknown) {
return get(this, key, toShallow)
return get(this, key, false, true)
},
get size() {
return size((this as unknown) as IterableCollections)
Expand All @@ -258,12 +260,14 @@ const shallowInstrumentations: Record<string, Function> = {

const readonlyInstrumentations: Record<string, Function> = {
get(this: MapTypes, key: unknown) {
return get(this, key, toReadonly)
return get(this, key, true)
},
get size() {
return size((this as unknown) as IterableCollections)
return size((this as unknown) as IterableCollections, true)
},
has(this: MapTypes, key: unknown) {
return has.call(this, key, true)
},
has,
add: createReadonlyMethod(TriggerOpTypes.ADD),
set: createReadonlyMethod(TriggerOpTypes.SET),
delete: createReadonlyMethod(TriggerOpTypes.DELETE),
Expand Down

3 comments on commit 50adc01

@jods4
Copy link
Contributor

@jods4 jods4 commented on 50adc01 Aug 8, 2020

Choose a reason for hiding this comment

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

I think this is mostly a good change, but it should be pointed out as a breaking change in the next release notes.
Previously wrapping the raw collection with readonly would work as a reactive object.
I'm saying "mostly" and I believe some people may have wrapped the raw collection because of a single drawback: it may seem more efficient to wrap the collection directly rather than stacking a proxy on top of another proxy.

Which makes me wonder: wouldn't it be more efficient to implement this behavior by proxying directly the raw collection? You could still use a different proxy if it was wrapping a reactive proxy (track) or not (don't track).

Speaking of efficiency, don't you think that at this point it would be better to split collections handlers completely? It would simplify both implementation as the (non-tracking) readonly handlers are now drastically simpler. Unless I'm wrong:

  • has, size and foreach don't need a proxy trap anymore, they can just passthrough.
  • get is massively simpler: (a) try an unwrapped key + (b) return toReadonly(value).

@jods4
Copy link
Contributor

@jods4 jods4 commented on 50adc01 Aug 9, 2020

Choose a reason for hiding this comment

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

After giving it more thinking, I changed my mind about the unwrapping of keys in a reactive/readonly Map or Set.

TL;DR: I think reactive collections should not unwrap keys, they should work the same way normal collections would (minus the reactivity)

Yes, proxies create referential problems and we know them well (==, Array:includes, Map, Set, etc.).
That's bad but at least we can easily explain it.
In a plain Map, a reactive collection would be a different key than a raw collection.
There's nothing we can do about that, it's how JS works.
That a reactive Map would work differently only adds to the confusion.
Reactive collections should work like their standard counterparts.

It's also dangerous.
When refactoring code, one should be able to replace a reactive collection by a plain one, and vice-versa, without having to fear the introduction of bugs.
With this automatic unwrapping, changing a collection to/from reactive has the potential to merge/split keys and create unexpected changes in behavior.

Similarly, when writing generic library code, we shouldn't have to ask ourselves if the Map we take as a parameter is reactive or not.
It's behavior should be clear: it's a Map and it works like one.

@jods4
Copy link
Contributor

@jods4 jods4 commented on 50adc01 Sep 13, 2020

Choose a reason for hiding this comment

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

@yyx990803 did you see my comments in this commit?
Most of it is minor tweaks, but I think the ones impacting public API should be changed before RTM.
In particular:

  • Set::add() and Map::set() break reactivity when chained.
  • Reactive collections behave differently from plain collections when their key is a reactive object.

And maybe:

  • Reactive collections don't invoke "overriden" methods on instances.

Please sign in to comment.