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(runtime-core): allow spying on proxy methods #4216

Merged
merged 1 commit into from Feb 12, 2022

Conversation

cexbrayat
Copy link
Member

@cexbrayat cexbrayat commented Jul 30, 2021

Since Jest v26.6.1, the mock method changed (see this commit jestjs/jest@30e8020) to rely on Object.defineProperty in some cases.

This breaks spying on proxy's methods, because even if Jest is properly calling Object.defineProperty, the cached value in the get section of the proxy is never updated, and the spy is in fact never used.
This is easily reproducible as vue-next already uses a version of jest with these changes.

This is blocking projects (like vue-test-utils-next and vue-cli) to update to recent Jest versions.

This commit adds a defineProperty method to the proxy handler, that properly updates the defined value in the cache.

Again, the fix is maybe too naive, and I'm happy to update the PR with a better solution.

edit: the same happens with Vitest

@cexbrayat
Copy link
Member Author

This is now rebased on top of #4472

Would be glad to hear your thoughts @yyx990803 as landing this would unblock the ecosystem to move to jest v27

@cexbrayat cexbrayat force-pushed the fix/spy-on-proxy branch 2 times, most recently from 9f23312 to 2d063f8 Compare September 2, 2021 09:54
@cexbrayat
Copy link
Member Author

Now that #4472 landed, this PR is rebased on master and ready to review.

The fix is intended to unblock the migration of the ecosystem to jest 27, but it also fixes a more general issue with updating proxies via Object.defineProperty.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

I hope we can merge this soon!

@LinusBorg LinusBorg added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. ready to merge The PR is ready to be merged. labels Dec 5, 2021
Since Jest v26.6.1, the mock method changed (see this commit jestjs/jest@30e8020)  to rely on `Object.defineProperty` in some cases.

This breaks spying on proxy's methods, because even if Jest is properly calling `Object.defineProperty`, the cached value in the `get` section of the proxy is never updated, and the spy is in fact never used.
This is easily reproducible as vue-next already uses a version of jest with these changes.

This is blocking projects (like vue-test-utils-next and vue-cli) to update to recent Jest versions.

This commit adds a `defineProperty` method to the proxy handler, that properly updates the defined value in the cache.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. ready to merge The PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants