search

parse-community / Parse-SDK-JS

The JavaScript SDK for Parse Platform
https://parseplatform.org
Apache License 2.0
1.33k stars 597 forks source link

fix: Local state wrong on multiple set operations on same property #2116

Closed mortenmo closed 6 months ago

mortenmo commented 6 months ago

Pull Request

Issue

Closes: https://github.com/parse-community/Parse-SDK-JS/issues/2115 Closes: https://github.com/parse-community/Parse-SDK-JS/pull/1451

Approach

When local state has multiple set/unset ops on the same base property, use the result of the previous instead of always going back to the server version for every Op to allow them to chain.

The test included fails until the fix is in place.

Tasks

parse-github-assistant[bot] commented 6 months ago

I will reformat the title to use the proper commit message syntax.

parse-github-assistant[bot] commented 6 months ago

Thanks for opening this pull request!

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (72bc9ac) to head (da3121c). Report is 15 commits behind head on alpha.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## alpha #2116 +/- ## =========================================== + Coverage 99.98% 100.00% +0.01% =========================================== Files 61 64 +3 Lines 6185 6208 +23 Branches 1499 1504 +5 =========================================== + Hits 6184 6208 +24 + Misses 1 0 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dplewis commented 6 months ago

@mtrezza @mortenmo A PR for this has already been opened. Try copying the test over from that PR. I'll review.

https://github.com/parse-community/Parse-SDK-JS/pull/1451

mortenmo commented 6 months ago

I really need to check existing PRs better :) . The fixes looks basically identical, so feel free to commit that one and I can close this one. I added the #1450 test case here just to be sure and it does also pass.

mstniy commented 6 months ago

Hey. I made some changes to #1451 . The patch also handles doubly-nested objects now, it used to be still broken in that case. I also modified the regression test to also include this scenario.

mtrezza commented 6 months ago

Is this PR made obsolete by https://github.com/parse-community/Parse-SDK-JS/pull/1451? If not, which one should be merged first, in case order matters?

mstniy commented 6 months ago

I believe so. I copied the new regression test in this branch over to my branch, and it does pass there also.

I would suggest #1451 to be merged in favor of this one, as that one implements a more robust deep cloning logic after today's changes, evidenced by the regression test here passing there, but not vica versa.

mtrezza commented 6 months ago

Superseded by https://github.com/parse-community/Parse-SDK-JS/pull/1451