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

Increase CRDT Counter in local change #441

Merged
merged 1 commit into from
Jan 25, 2023
Merged

Increase CRDT Counter in local change #441

merged 1 commit into from
Jan 25, 2023

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Jan 19, 2023

What this PR does / why we need it?

Any background context you want to provide?

What are the relevant tickets?

Fixes #438

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@codecov
Copy link

codecov bot commented Jan 19, 2023

Codecov Report

Merging #441 (189924e) into main (d67e898) will decrease coverage by 0.10%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   89.51%   89.41%   -0.10%     
==========================================
  Files          69       69              
  Lines        5321     5328       +7     
  Branches      524      530       +6     
==========================================
+ Hits         4763     4764       +1     
  Misses        381      381              
- Partials      177      183       +6     
Impacted Files Coverage Δ
test/integration/counter_test.ts 86.95% <0.00%> (-8.29%) ⬇️
src/document/json/counter.ts 78.94% <100.00%> (+1.16%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cozitive cozitive marked this pull request as ready for review January 25, 2023 05:27
@hackerwins
Copy link
Member

hackerwins commented Jan 25, 2023

@cozitive

There was some confusion in the documentation description.

  1. Update by 1.1 Doc.Update() refers to the operation of this function. It seems that Root(Proxy) in the document should be changed to Clone(Proxy).

  2. Update by 1.3 Apply Changes refers to the operation of applying operations logic. I think we need to fix the CRDT to Root.

You can check the modified diagram at the link below.
https://miro.com/app/board/o9J_kidjCtk=/?moveToWidget=3458764544271491642&cot=14

@hackerwins hackerwins self-requested a review January 25, 2023 07:08
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

@chacha912 , @cozitive
Thanks for your contribution.

test/integration/counter_test.ts Show resolved Hide resolved
@hackerwins hackerwins merged commit f7be9e7 into yorkie-team:main Jan 25, 2023
hunkim98 pushed a commit to hunkim98/yorkie-js-sdk that referenced this pull request Jul 12, 2023
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.

Local change is not updating in the counter example
3 participants