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

wants to merge 12 commits into from


Copy link

close #2619

Copy link

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:, '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).

Copy link
Member Author

edison1105 commented Nov 17, 2020, 'a')

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

LinusBorg previously requested changes Nov 20, 2020
Copy link

@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
Copy link
Member Author

Can you add a test?


Copy link

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, 'a') works.


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.

Copy link

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

Copy link

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
Copy link

netlify bot commented Nov 14, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit fe7a0a8
🔍 Latest deploy log

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
🍰 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.
None yet

Successfully merging this pull request may close these issues.

hasOwnProperty is not tracked
5 participants