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

Enhance Set and Add for representing nested elements #691

Merged
merged 11 commits into from
Nov 23, 2023

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Nov 20, 2023

What this PR does / why we need it?

When setting values for objects or arrays, we first created the outer shells of objects ({}) and arrays ([]), and then individually set the inner values. As a result, multiple operations were generated. This PR introduces a modification that allows setting initial values directly within a single operation, simplifying the process and reducing the number of generated operations.

Consider the example of setting the shape of a circle:

doc.update((root) => {
  root.shape = {};
});

doc.update((root) => {
  root.shape.circle = {
    point: { x: 0, y: 0 },
    color: 'red',
  };
});

doc.update((root) => {
  root.shape.circle = {
    point: { x: 1, y: 1 },
    color: 'blue',
  };
});

as-is

image

In this case, the generated operations are 1, 5, and 5, respectively. Examining the undo operations in the final state reveals that each set operation is followed by a remove operation. Finally, a set operation is performed to restore the previous state of circle = {point: { x: 0, y: 0 }, color: 'red'}. However, due to the current set operation's limitation in setting only outer shells, it results in improper behavior.

to-be

This PR enhances the set operation to handle both outer shells and inner values properly.
So it looks like the set operation is only generated once, and undo operations are applied correctly.

image

Any background context you want to provide?

What are the relevant tickets?

Fixes yorkie-team/yorkie#663

Checklist

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

…ect and array

Previously, only the outer shells of object ({}) and array ([]) were added,
requiring separate operations to set their internal values. The modification
enables setting initial values directly within a single operation.
hackerwins pushed a commit to yorkie-team/yorkie that referenced this pull request Nov 21, 2023
This commit is associated with yorkie-team/yorkie-js-sdk#691. Since
modifications have been made to allow setting values in set and add
operations, it is necessary to update the converter accordingly. This
involves enabling the setting of values for objects and arrays in the
converter.
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

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

Comparison is base (5b1425d) 68.01% compared to head (5f6fffe) 69.12%.

Files Patch % Lines
src/api/converter.ts 50.00% 4 Missing and 3 partials ⚠️
src/document/operation/remove_operation.ts 78.57% 1 Missing and 2 partials ⚠️
src/document/json/element.ts 88.23% 1 Missing and 1 partial ⚠️
src/document/operation/set_operation.ts 83.33% 0 Missing and 2 partials ⚠️
src/document/operation/add_operation.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #691      +/-   ##
==========================================
+ Coverage   68.01%   69.12%   +1.10%     
==========================================
  Files          58       58              
  Lines        8776     8787      +11     
  Branches      793      800       +7     
==========================================
+ Hits         5969     6074     +105     
+ Misses       2548     2454      -94     
  Partials      259      259              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hackerwins hackerwins changed the title Enhance Set and Add operations to include initial values Enhance Set and Add for representing nested elements Nov 23, 2023
@hackerwins hackerwins merged commit ed35072 into main Nov 23, 2023
2 checks passed
@hackerwins hackerwins deleted the set-nested-object branch November 23, 2023 10:18
@hackerwins
Copy link
Member

@chacha912 Thanks for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the set operation to handle nested object values
2 participants