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

Change the value of XXXChange to Change in Document.subscribe #538

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

easylogic
Copy link
Contributor

What this PR does / why we need it?

It's reduce changes loop when local-change and remote-change are fired

Any background context you want to provide?

  • Reduce the number of loops to make the CRDT changes more obvious.

What are the relevant tickets?

Fixes #537

Checklist

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

@codecov
Copy link

codecov bot commented Jun 7, 2023

Codecov Report

Merging #538 (f9badc9) into main (d38c853) will decrease coverage by 0.02%.
The diff coverage is 97.43%.

@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
- Coverage   88.10%   88.08%   -0.02%     
==========================================
  Files          77       77              
  Lines        7397     7386      -11     
  Branches      714      713       -1     
==========================================
- Hits         6517     6506      -11     
  Misses        607      607              
  Partials      273      273              
Impacted Files Coverage Δ
test/integration/text_test.ts 97.66% <91.66%> (-0.11%) ⬇️
src/document/document.ts 73.24% <100.00%> (-0.17%) ⬇️
test/integration/client_test.ts 99.78% <100.00%> (ø)
test/integration/document_test.ts 98.98% <100.00%> (ø)
test/unit/document/document_test.ts 98.04% <100.00%> (ø)

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.

Thanks for your contribution.
Would it be possible for you to test all the examples too? It appears that there might be a small typo in public/editor.html.

public/editor.html Outdated Show resolved Hide resolved
Copy link
Contributor

@chacha912 chacha912 left a comment

Choose a reason for hiding this comment

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

It looks good :)

src/document/document.ts Show resolved Hide resolved
@hackerwins hackerwins changed the title reduce changes loop Change the value of XXXChange to Change in Document.subscribe Jun 8, 2023
@hackerwins hackerwins self-requested a review June 8, 2023 03:37
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.

Thanks for your contribution.

@hackerwins hackerwins merged commit 8151e8a into main Jun 8, 2023
2 checks passed
@hackerwins hackerwins deleted the feat/reduce-changes-loop branch June 8, 2023 03:49
hunkim98 pushed a commit to hunkim98/yorkie-js-sdk that referenced this pull request Jul 12, 2023
…-team#538)

Previously, we passed ChangeInfos as values of RemoteChange and
LocalChange types in Document.subscribe, and users should iterate
loop to extract Change. This commit simply passes Change instead of
Changes to remove changes loop in Document.subscribe.

---------

Co-authored-by: Youngteac Hong <susukang98@gmail.com>
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.

Reducing the changes loop
3 participants