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

readonly() breaks reactivity of Map #1772

Closed
0x009922 opened this issue Aug 4, 2020 · 7 comments
Closed

readonly() breaks reactivity of Map #1772

0x009922 opened this issue Aug 4, 2020 · 7 comments

Comments

@0x009922
Copy link

0x009922 commented Aug 4, 2020

Version

3.0.0-rc.5

Reproduction link

https://codepen.io/liquid-solid/pen/ZEWzqpX?editors=0012

Steps to reproduce

Open CodePen and see console logs, or you can see an example below:

const {
  readonly,
  reactive,
  isReactive
} = Vue

const map = reactive(new Map())
const roMap = readonly(map)

map.set(4, { foo: 'bar' })

const mapItem = map.get(4)
const roMapItem = roMap.get(4)

console.log(`
Is map reactive?         - ${isReactive(map)}
Is map item reactive?    - ${isReactive(mapItem)}
Is RO map reactive?      - ${isReactive(roMap)}
Is RO map item reactive? - ${isReactive(roMapItem)}
`)

This code outputs:

Is map reactive?         - true
Is map item reactive?    - true
Is RO map reactive?      - true
Is RO map item reactive? - false

What is expected?

I expect that objects from a reactive Map will always be reactive, even if the Map is readonly.

What is actually happening?

When I get an item from readonly reactive Map it loses reactivity.


Is it a bug? If not, i don't understand how to work with maps, sets and other structs with new reactivity system. So far, plain objects still seem to be the best solution for storing data as an associative array due to the intricacies of the reactivity system.

@unbyte
Copy link
Contributor

unbyte commented Aug 5, 2020

Items from a readonly map will be wrapped with toReadonly so it cannot be edit:
https://github.com/vuejs/vue-next/blob/a0e34cee4a09a14548bf1e78f4a82702e9d40717/packages/reactivity/src/collectionHandlers.ts#L255-L268

and in https://composition-api.vuejs.org/api.html#readonly it says

A readonly proxy is deep: any nested property accessed will be readonly as well.

so it's not a bug I think.

@0x009922
Copy link
Author

0x009922 commented Aug 5, 2020

@unbyte I know that readonly works deeply. This is the desired behavior, I really need me to pass the map to other parts of the application as read-only. The problem is that nested objects in the map lose their reactivity.
I have prepared another example on CodePen, or you can see it right here:

const {
  readonly,
  reactive,
  watchEffect
} = Vue;

const map = reactive(new Map())
const roMap = readonly(map);

// Sync flush for debug purposes
const watchEffectSync = (callback) => watchEffect(callback, { flush: 'sync' })

// Try with default object
{
  console.log('--- Default object')
  map.set(0, { foo: 'bar' });
  const item = map.get(0);
  const roItem = roMap.get(0);

  watchEffectSync(() => {
    console.log('Watch effect item:', item);
  })
  watchEffectSync(() => {
    console.log('Watch effect ro item:', roItem);
  })

  console.log('Mutating item...');

  item.foo = 'Updated value';

  console.log('Ro item (not in watchEffect):', roItem);
}

// Try with reactive object
{
  console.log('--- Reactive object')
  map.set(1, reactive({ foo: 'bar' }));
  const item = map.get(1);
  const roItem = roMap.get(1);

  watchEffectSync(() => {
    console.log('Watch effect item:', item);
  })
  watchEffectSync(() => {
    console.log('Watch effect ro item:', roItem);
  })

  console.log('Mutating item...');

  item.foo = 'Updated value';

  console.log('Ro item (not in watchEffect):', roItem);
}

Console output:

"--- Default object"
"Watch effect item:" Object {
  foo: "bar"
}
"Watch effect ro item:" Object {
  foo: "bar"
}
"Mutating item..."
"Watch effect item:" Object {
  foo: "Updated value"
}
"Ro item (not in watchEffect):" Object {
  foo: "Updated value"
}
"--- Reactive object"
"Watch effect item:" Object {
  foo: "bar"
}
"Watch effect ro item:" Object {
  foo: "bar"
}
"Mutating item..."
"Watch effect item:" Object {
  foo: "Updated value"
}
"Ro item (not in watchEffect):" Object {
  foo: "Updated value"
}

As you can see, the watchEffect of an object from a read-only map never fires again when the object changes, but when the roItem itself is displayed, there is a change, therefore, reactivity does not work.

The point is not how isReactive works, but that roItem is really not reactive.


Also, isReactive doesn't true when item is explicit reactive:

const map = reactive(new Map())
map.set(0, reactive({ foo: 'bar' }))
console.log(isReactive(readonly(map).get(0))) // => false

@unbyte
Copy link
Contributor

unbyte commented Aug 5, 2020 via email

@0x009922
Copy link
Author

0x009922 commented Aug 5, 2020

@unbyte I think you did not quite understand me. In your example, it goes like this:

// Create some raw object
const test = { some: 'value' };

// Create reactive wrapper of RAW test
const reactiveTest = reactive(test)

// Create readonly wrapper of RAW test
const readonlyTest = readonly(test)

// Mutating object via reactive wrapper
reactiveTest.some = 'new value'

And in my example, it works a little differently:

// Create some raw object
const test = { some: 'value' };

// Create reactive wrapper of RAW test
const reactiveTest = reactive(test)

// Create readonly wrapper of REACTIVE test
const readonlyTest = readonly(reactiveTest)
//                            ^^^^^^^^^^^^

// Mutating object via reactive wrapper
reactiveTest.some = 'new value'

I am making readonly wrapper under reactive wrapper of object, not plain object. And in this case, when test is a plain object, reactivity works:

watchEffect(() => console.log('Value of reactive:', reactiveTest.some));
watchEffect(() => console.log('Value of readonly(reactive):', readonlyTest.some));

// mutating...
reactiveTest.some = 'new value';

Output:

"Value of reactive:" "value"
"Value of readonly(reactive):" "value"
"Value of reactive:" "new value"
"Value of readonly(reactive):" "new value"

As you can see, the reactivity of a plain object is not lost when wrapping it in readonly.

@unbyte
Copy link
Contributor

unbyte commented Aug 5, 2020

Finally I understand what you mean. I'm so foolish.

But the items got from maps are not like this

const reactiveTest = reactive(test)
const readonlyTest = readonly(reactiveTest)

Instead, they are more like this

const reactiveTest = reactive(test)
const readonlyTest = readonly(test)

because readonly(reactive) works on not items but map itself.

and of course readonly(reactive) works well if it's used on items as below

  const {
    readonly,
    reactive,
    watchEffect
  } = Vue

  const map = reactive(new Map())
  const roMap = readonly(map)

  const watchEffectSync = (callback) => watchEffect(callback, { flush: 'sync' })

  map.set(0, { foo: 'bar' })
  const item = map.get(0)
  const roItem = readonly(item)
  //             ^^^^^^^^^^^^^

  watchEffectSync(() => {
    console.log('Watch effect item:', item.foo, item)
  })
  watchEffectSync(() => {
    console.log('Watch effect ro item:', roItem.foo, roItem)
  })

  console.log('Mutating item...')

  item.foo = 'Updated value'

  console.log('Ro item (not in watchEffect):', roItem)

// both watcher callbacks are triggered

Go back to the problem that you really want to solve (I'm so sorry, I seem to have taken the focus of the problem off center), how about using useXXX function instead of pass a readonly map?

   function deepClone(obj = {}) {
    if (typeof obj !== 'object' || obj == null)
      return obj
    const result = obj instanceof Array ? [] : {}
    for (const key in obj)
      if (obj.hasOwnProperty(key))
        result[key] = obj[key]
    return result
  }

  const {
    readonly,
    reactive,
    watchEffect
  } = Vue

  const map = reactive(new Map())

  const watchEffectSync = (callback) => watchEffect(callback, { flush: 'sync' })

  map.set(0, { foo: 'bar' })

  function useMap() {
    // and return size, entries ...
    return {
      getMapItem(key) {
        return deepClone(map.get(key))
      }
    }
  }

  some()

  function some() {
    const { getMapItem } = useMap()
    watchEffectSync(() => {
      const value = getMapItem(0)
      console.log(value)
      value.foo = 'another'
    })
  }

  map.get(0).foo = 'hello'

@0x009922
Copy link
Author

0x009922 commented Aug 5, 2020

@unbyte Hmm. I will try to describe my case more precisely. Here's another example on CodePen.

In short, it has the following functions for creating a repository:

function useUnsafeStore() {
  const state = reactive({
    users: new Map([
      ['default', { name: 'initial value' }]
    ])
  });

  // `reactive` for computed refs unwrapping
  const getters = reactive({
    usersIds: computed(() => [...state.users.keys()])
  })

  const mutations = {
    updateUserName(id, value) {
      state.users.get(id).name = value;
    }
  }

  return {
    state,
    getters,
    mutations
  }
}

function useReadonlyStore() {
  return readonly(useUnsafeStore());
}

The useUnsafeStore function gives the state, getters and mutations as they are, and nothing prevents you from changing the state of the store (or even replacing mutations) from outside this store, which is not good. The useReadonlyStore function returns the same store wrapped from head to toe (deeply) in a readonly Proxy, which is intended to ensure that the store is not modified from the outside.

Next, I create two different applications: one with unsecured storage, the other with readonly, and, as you can see from the example, reactivity in the second case is lost, although the element itself inside the Map does change, as can be seen after applying $forceUpdate.

@unbyte
Copy link
Contributor

unbyte commented Aug 6, 2020

I have created a pull request to solve this problem. 😀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants