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

DDS to use deep-cloning / immutable objects when raising events, returning current state #5738

Closed
vladsud opened this issue Apr 7, 2021 · 2 comments

Comments

@vladsud
Copy link
Contributor

vladsud commented Apr 7, 2021

Let's consider a case of ISharedMap (as an example) and a value stored in it being an object (any mutable object, like an array, or object with properties).

There are 3 key workflows:

  1. Setting new value for a key:
   const value = {a: 6, b: 7};
   map.set("mydata", value);
   value.a = 5;
  1. reading data:
   const value = map.get("mydata");
   value.a = 4;
  1. get, modify, and set data:
   value = map.get("mydata");
   value.a = 5;
   map.set("mydate", value);

Requirements:

  1. After value is set on DDS, original object stays mutable. I.e. it's not deep-frozen. Making any changes on this object has no impact on a value stored in DDS.
  2. It's impossible to change DDS value by mutating value retrieved from it (case # 2 above).
  3. Convenience (optional, but highly desired): get-modify-set cycle should be easy to use, ideally without a need to deep-clone an object retrieved from DDS in order to mutate it.

Today, first two scenarios are broken.
While third one sort of works, but really broken as value change event is incorrect (it would say that nothing changed).

There are 3 possible mechanisms we could use here:

  1. Deep freezing objects. This does not work well for set & get-modify-set scenarios.
  2. Deep readonly types - this only works for get scenario. Does not help at all with set and makes user dep clone data for get-modify-set
  3. Deep cloning on get & set. This works pretty well from usability and correctness points of view.

So it looks like deep clone is the only possible solution here.
Please note that from implementation perspective, it does not have to be full clone.
Libraries like Immer and immutable.js could be used to optimize process here.

As for API surface, that implies to everything, including change event notifications - we should never ever expose internal data structures for user to be able to directly modify them.

@ghost ghost added the triage label Apr 7, 2021
@tylerbutler tylerbutler added the design-required This issue requires design thought label Apr 18, 2021
@tylerbutler tylerbutler added this to the May 2021 milestone Apr 18, 2021
@ghost ghost removed the triage label Apr 18, 2021
@tylerbutler tylerbutler added api and removed design-required This issue requires design thought labels May 24, 2021
@vladsud vladsud changed the title Object Lifetime management when using DDSs DDS to use deep-cloning / immutable objects when raising events, returning current state May 26, 2021
@anthony-murphy anthony-murphy modified the milestones: May 2021, June 2021 Jun 1, 2021
@vladsud vladsud modified the milestones: June 2021, Future Jun 9, 2021
@microsoft-github-policy-service
Copy link
Contributor

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

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

No branches or pull requests

3 participants