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

markRaw / shallowRef is not respected by deep watch #2380

Open
mannok opened this issue Oct 14, 2020 · 9 comments
Open

markRaw / shallowRef is not respected by deep watch #2380

mannok opened this issue Oct 14, 2020 · 9 comments

Comments

@mannok
Copy link

mannok commented Oct 14, 2020

Version

3.0.0

Reproduction link

https://github.com/mannok/vuex-traverse

Steps to reproduce

  1. clone from minimal reproduction link
  2. npm install
  3. open App.vue to see code
  4. npm run dev
  5. Open browser and watch console log
    6-a. You will see a log msg THIS MSG IS SHOWN WHEN IT WAS SET ON STATE! even I have marked raw to the object. i.e. log msg shouldn't be shown, as markRaw(obj)'s properties shouldn't be traversed down when it was set to state.
    6-b. Comment line 30 and comment line 31 and refresh the browser again. You will no longer see the log msg as Vue won't traverse down markRaw(obj).

What is expected?

When I call markRaw() for any object. It shouldn't be traversed in any scenario. (Vue or Vuex)

What is actually happening?

It is acutally traversing when the marked raw object is set on Vuex state.


Maybe you will think this problem doesn't matter, but currently I am using a big library which comprises of many complex object. I just want to store them inside state as raw object / shallowRef, but I have no way to do it... It keeps traversing deeply for me and the performance is really bad... Please help me out! Thanks in advance!

UPDATE
Seems that my problem is that I have no way to stop Vuex to run traverse() inside vue\dist\vue.runtime.global.js. FYR:

  function traverse(value, seen = new Set()) {
      if (!isObject(value) || seen.has(value)) {
          return value;
      }
      seen.add(value);
      if (isRef(value)) {
          traverse(value.value, seen);
      }
      else if (isArray(value)) {
          for (let i = 0; i < value.length; i++) {
              traverse(value[i], seen);
          }
      }
      else if (isMap(value)) {
          value.forEach((_, key) => {
              traverse(value.get(key), seen);
          });
      }
      else if (isSet(value)) {
          value.forEach(v => {
              traverse(v, seen);
          });
      }
      else {
          for (const key in value) {
              traverse(value[key], seen);  // Vuex recursively loop all my raw object properties in this line
          }
      }
      return value;
  }
@LinusBorg
Copy link
Member

LinusBorg commented Oct 14, 2020

Edit: Oh I see you came to the same conclusion ... 😊

That's because strict: true walks the entire state tree order to enforce warn if the state as mutated outside of a mutation.

My first instinct is to rate this s bug: deep: true should not traverse objects marked as raw, I'd rate this a bug.

But thinking about it, one could argue that it should traverse them as a raw object could still contain nested reactive objects. If it didn't, and a raw object contains a reactive opbject, it's mutation could not be observed by strict mode. Then again, mutations to the raw object itself wouldn't be observed either way, so the whole thing leaks anyway.

So we consider it a bug, the fix is easy. traverse should simply bail when the object has ReactiveFlas.SKIP set.

https://github.com/vuejs/vue-next/blob/c6443a43c9744d8d40f7129a98792debe07a3a82/packages/runtime-core/src/apiWatch.ts#L325

Edit: Not that easy for shallowReactive as that object or rather nested objects aren't necessarily marked. maybe we just need to bail from traversing when an object is !isReactive(obj)?

If we don't, then we need to discuss wether we should have a second way of a deep watch that does skip raw-marked objects, like deep: 'shallow' or something?

@LinusBorg LinusBorg changed the title markRaw / shallowRef is not working in vuex markRaw / shallowRef is not respected by deep watch Oct 14, 2020
@mannok
Copy link
Author

mannok commented Oct 14, 2020

Edit: Oh I see you came to the same conclusion ... 😊

That's because strict: true walks the entire state tree order to enforce warn if the state as mutated outside of a mutation.

My first instinct is to rate this s bug: deep: true should not traverse objects marked as raw, I'd rate this a bug.

But thinking about it, one could argue that it should traverse them as a raw object could still contain nested reactive objects. If it didn't, and a raw object contains a reactive opbject, it's mutation could not be observed by strict mode. Then again, mutations to the raw object itself wouldn't be observed either way, so the whole thing leaks anyway.

So we consider it a bug, the fix is easy. traverse should simply bail when the object has ReactiveFlas.SKIP set.

https://github.com/vuejs/vue-next/blob/c6443a43c9744d8d40f7129a98792debe07a3a82/packages/runtime-core/src/apiWatch.ts#L325

Edit: Not that easy for shallowReactive as that object or rather nested objects aren't necessarily marked. maybe we just need to bail from traversing when an object is !isReactive(obj)?

If we don't, then we need to discuss wether we should have a second way of a deep watch that does skip raw-marked objects, like deep: 'shallow' or something?

Thanks so much @LinusBorg . You have saved me several times at Vue 2 before and now Vue 3.

Yes, I have thought that there may be some cases that a raw object contains a reactive object. I think you are right that Vue should take care those nested reactive object mutation inside strict mode. However, one thing I am quite confuse. According to Vuex doc, strict is to gaurantee that nothing (not just reactive objects) in state tree could be mutated from outside, either for raw object or reactive object. Therefore,

If it didn't, and a raw object contains a reactive opbject, it's mutation could not be observed by strict mode.

this statement should not only for rawObj.reactiveObj, but even for rawObj.rawObj. i.e. strict mode should observe all mutation inside state tree, even for nested raw object. Did I interpret your words wrong?

@LinusBorg
Copy link
Member

LinusBorg commented Oct 14, 2020

a raw object is not reactive, and can't be observed (that's why you marked it raw, right?).

Strict mode just watches the state tree for changes to reactive objects, and then warns about them. changes to nonreactive objects can't be observed and consequently, can't be warned about.

Vuex should likely add an option to just wrap the exposed $state in readonly() as an alternative

@jods4
Copy link
Contributor

jods4 commented Jan 10, 2021

@LinusBorg

But thinking about it, one could argue that it should traverse them as a raw object could still contain nested reactive objects.

That's exactly what I would argue.

I may create a shallowReactive array because I don't want to deeply proxify everything that is in it.
It doesn't mean that the objects inside the array can't have one property that is reactive, e.g. { todo: "learn vue", active: ref(true) }.
Then I might want to deeply observe the array, which would only take a dependency on the active refs (all the rest being non-reactive).

For me the semantics of "shallow" apply to automatic proxifying (proxification?) of the whole object graph. They are unrelated to observation itself.

Consider that a watchEffect would observe everything touched by its code (even indirectly through opaque functions), even past "shallow" boundaries.
IMHO it would be inconsistent if watch with deep: true would act differently.

a raw object is not reactive, and can't be observed (that's why you marked it raw, right?).

Someone who's doing advanced stuff may use markRaw to avoid Vue wrapping his objects automatically, and create his own reactive objects based on @vue/reactivity.

That kind of scenarios would be totally broken if the change suggest here is done.

@jacekkarczmarczyk
Copy link
Contributor

should there be markRaw and markDeepRaw or markRaw(object, { deep: bool })?

@jods4
Copy link
Contributor

jods4 commented Jan 10, 2021

I wonder how that would work.

By essence markRaw means: don't put this into a proxy, so I don't see how it would be able to intercept everything that's put into it to mark it as well?

@jods4
Copy link
Contributor

jods4 commented Jan 10, 2021

BTW I'm not sure what is the exact use case here.
The built-in deep watch means: when anything reactive in that state graph, at any level, changes, then run the watch again.

If you want something more specialized, e.g. maybe just 1 or 2 level deep, or to any other arbitrary condition (e.g. the object is marked as raw), then I think the proper way is to do your own traversal and not rely on the built-in deep.

It's easy to do today in user-land and it's more healthy for Vue to keep general, intuitive semantics for the built-in functions.

@waruyama
Copy link

waruyama commented Mar 1, 2022

@jods4 Can you provide a hint how to implement custom traversal?

Background: I intenden to use shallowReactive() for an array of points that changes rapidly. Each point references a rather deep object that is not meant to be reactive, so performance suffered quite a bit. All I am interested in is if the entries in the array change, everything below that does not concern me.

@jods4
Copy link
Contributor

jods4 commented Mar 2, 2022

@waruyama There are a few quirks.

👉 I feel like there's one option missing from the watch API here. When your source is a reactive object or array it's hardcoded to always be observed deeply. Seems to me it should be possible to watch shallowly if you don't ask for deep.

👉 deep has the side-effect of running the callback whenever the dependencies are invalidated, no matter what's the return value of source. When you don't use deep, callback only runs if the value returned by source has changed.

But you can work around things by fiddling with the source function.

// If by "the entries in the array change" you mean: adding/removing/assigning the array itself, this should work:
watch(
  () => { 
    array.forEach(() => {});   // read all array entries
    return {}; // return a new instance as the "value" so that it's different and callback runs
  },
  callback);

// If you mean the points themselves can mutate and you want to shallowly observe them:
```js
watch(
  () => {
    array.forEach(Object.entries); // read all properties of every point
    return {}; // as before: new "value" each time something changes
  },
  callback);  

That's the gist of it, you can tweak the source function in many ways.

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

No branches or pull requests

5 participants