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: update vue 2 reactivity #1773

Merged
merged 6 commits into from
May 1, 2024

Conversation

Graphmaxer
Copy link
Contributor

@Graphmaxer Graphmaxer commented Feb 1, 2024

πŸ”— Linked issue

#1772

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

When updating an object in Vue 2, we cannot use directly Object.assign with 2 params and delete is not reactive.

cf. https://v2.vuejs.org/v2/guide/reactivity.html#For-Objects

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov-commenter
Copy link

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (a7ab3bf) 99.10% compared to head (b289c7b) 98.94%.
Report is 5 commits behind head on main.

Files Patch % Lines
...kages/pinia-orm/src/composables/useStoreActions.ts 70.58% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1773      +/-   ##
==========================================
- Coverage   99.10%   98.94%   -0.17%     
==========================================
  Files          90       90              
  Lines        5917     5946      +29     
  Branches      494      499       +5     
==========================================
+ Hits         5864     5883      +19     
- Misses         50       60      +10     
  Partials        3        3              

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@Graphmaxer
Copy link
Contributor Author

I don't know how to have 100% of code coverage with Vue 2 conditions, maybe it's possible to combine multiple reports with nyc https://github.com/istanbuljs/nyc?tab=readme-ov-file#combining-reports-from-multiple-runs

@Graphmaxer
Copy link
Contributor Author

Some tests are failing with test:2 :/

@CodeDredd
Copy link
Owner

@Graphmaxer Thanks for the PR. I try to look into it and think about it.
I don't like it to have a new package as dependency. I hopefully find a solution to your problem.
Meanwhile it would be good if could write a test for this.

@Graphmaxer
Copy link
Contributor Author

I updated the test performance/prevent_rerender_of_child_components to be compatible with Vue 2 and it is failing because of a lack of reactivity. This test is already testing the bug.

@Graphmaxer
Copy link
Contributor Author

I found a proper way without vue-demi as a direct dependency and added a specific tests to test reactivity and it is working great :)

@Graphmaxer Graphmaxer changed the title fix: use vue2 set and delete to update store state with reactivity fix: update vue 2 reactivity Feb 9, 2024
@phillipfickl
Copy link

Thanks for your PR, it works well for me, reactivity is working again! Tested on 2.7.16.

@Graphmaxer
Copy link
Contributor Author

Any update on the review @CodeDredd ?

@phillipfickl
Copy link

@CodeDredd if you found a minute to review and release this, I'd really appreciate it! Thanks for your time

@CodeDredd CodeDredd merged commit 676e9a0 into CodeDredd:main May 1, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants