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): fix track hasOwnProperty #2621

Closed
wants to merge 12 commits into from

Conversation

edison1105
Copy link
Member

close #2619

@skirtles-code
Copy link
Contributor

Correct me if I'm wrong but doesn't this fix assume that hasOwnProperty is being read off the reactive object itself?

If someone did something equivalent to this:

Object.prototype.hasOwnProperty.call(obj, 'a')

then the reactivity still wouldn't be tracked?

While it's unlikely that someone would do that directly in a template, it's pretty common for libraries to do something like this internally (like Vue's hasOwn helper).

@edison1105
Copy link
Member Author

edison1105 commented Nov 17, 2020

Object.prototype.hasOwnProperty.call(obj, 'a')

This will not be tracked and I am not sure that this PR is the best solution.

Copy link
Member

@LinusBorg LinusBorg left a comment

Choose a reason for hiding this comment

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

Can you add a test?

@LinusBorg LinusBorg added the need test The PR has missing test cases. label Nov 20, 2020
@edison1105
Copy link
Member Author

Can you add a test?

Done.

@HcySunYang
Copy link
Member

HcySunYang commented Jan 12, 2021

According to these specs:

  1. Assert: Type(O) is Object.
  2. Assert: IsPropertyKey(P) is true.
  3. Let desc be ? O.[[GetOwnProperty]](P).
  4. If desc is undefined, return false.
  5. Return true.

In step 3 O.[[GetOwnProperty]](P), which can be traped by getOwnPropertyDescriptor, something like this:

function getOwnPropertyDescriptor(target: object, key: string | number | symbol) {
  track(target, TrackOpTypes.GET, key)
  return Reflect.getOwnPropertyDescriptor(target, key)
}

This makes Object.prototype.hasOwnProperty.call(obj, 'a') works.

Edit:

Did some research, some basic semantics, such as [[Get]]/[[Set]]/[[Delete]] etc. all depend on O.[[GetOwnProperty]](P), this is where the devil is, making it impossible to intercept getOwnPropertyDescriptor to reach the goal.

@LinusBorg
Copy link
Member

@HcySunYang Do I read your last comment correctly: Are we stuck here?

@HcySunYang
Copy link
Member

Except for the solution given by this PR, I can't think of a better way.

@HcySunYang HcySunYang added the 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. label Mar 31, 2021
@LinusBorg LinusBorg dismissed their stale review January 21, 2022 16:28

Removing my change request as this need some guidance from Evan I think

@LinusBorg LinusBorg removed their request for review February 19, 2022 10:11
@netlify
Copy link

netlify bot commented Nov 14, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit fe7a0a8
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/6372032addb1d5000908bbf9

@yyx990803 yyx990803 closed this in 588bd44 Nov 14, 2022
@yyx990803
Copy link
Member

I noticed this fix is not granular: i.e. it would trigger even when unrelated keys are added or deleted.

More precise tracking can be achieved by returning an instrumented version of hasOwnProperty on reactive objects, as implemented in 588bd44.

chrislone pushed a commit to chrislone/core that referenced this pull request Feb 4, 2023
zhangzhonghe pushed a commit to zhangzhonghe/core that referenced this pull request Apr 12, 2023
nick-lai added a commit to nick-lai/petite-vue that referenced this pull request Nov 3, 2023
Avoid circular referencing the `hasOwnProperty` method in reactive proxy.

> vuejs/core#2621 (comment)
> More precise tracking can be achieved by returning an instrumented
> version of `hasOwnProperty` on reactive objects, as implemented in
> vuejs/core@588bd44
nick-lai added a commit to nick-lai/petite-vue that referenced this pull request Nov 6, 2023
Avoid circular referencing the `hasOwnProperty` method in reactive proxy.

> vuejs/core#2621 (comment)
> More precise tracking can be achieved by returning an instrumented
> version of `hasOwnProperty` on reactive objects, as implemented in
> vuejs/core@588bd44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. need test The PR has missing test cases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hasOwnProperty is not tracked
5 participants