freezed icon indicating copy to clipboard operation
freezed copied to clipboard

Support deep copy for Iterables / Maps

Open creativecreatorormaybenot opened this issue 4 years ago • 38 comments

It would be awesome if Freezed supported deep copies.

Example

Idea

For example, let us say I have the following two frozen classes:

@freezed
abstract class ContainerObject with _$ContainerObject {
  const factory ContainerObject(Map<String, Variable> variables) = _ContainerObject;
}

@freezed
abstract class Variable with _$Variable {
  const factory Variable.data() = Data;
  const factory Variable.tree() = Tree;
}
  1. I want to be able to generate a ContainerObject and then make a deep copy of it:
final containerObjectA = ContainerObject({...});

final containerObjectB = containerObjectA.copyWith();
  1. I want to manipulate the data of containerObjectB and then compare it to containerObjectA:
containerObjectB.variables['whatever'] = Variable.data();

final edited = containerObjectA == containerObjectB;

Problem

Step 2 technically works because I saw that you are making use of DeepEquality. The problem is that it fails because step 1 does not work afaic.
I realize that the idea here would be not edit the map and pass an altered map to copyWith. But how am I supposed to do that? Then, I am back to not having any benefit from freezed and it gets worse when using Lists.

I cannot know the Map keys ahead of time and if I had a list, there would be no way to represent it as a freezed value anyway.

Map.of

It might seem like that would be a solution here. There are two major problems with that:

  1. If I have to use Map.of manually, what is the benefit of freezed?

  2. Nesting lists and maps will make it extremely cumbersome to copy.

Common use case

What if I had the following instead:

@freezed
abstract class ContainerObject with _$ContainerObject {
  const factory ContainerObject(List<Variable> variables) = _ContainerObject;
}

This seems like a pretty common use case, but I would not be able to create a copy with freezed.

I don't understand the problem part at all.

Could you rephrase?

On Mon, 25 May 2020, 14:12 creativecreatorormaybenot, < [email protected]> wrote:

It would be awesome if Freezed supported deep copies. Example Idea

For example, let us say I have the following two frozen classes:

@freezedabstract class ContainerObject with _$ContainerObject { const factory ContainerObject(Map<String, Variable> variables) = _ContainerObject; } @freezedabstract class Variable with _$Variable { const factory Variable.data() = Data; const factory Variable.tree() = Tree; }

  1. I want to be able to generate a ContainerObject and then make a deep copy of it:

final containerObjectA = ContainerObject({...}); final containerObjectB = containerObjectA.copyWith();

  1. I want to manipulate the data of containerObjectB and then compare it to containerObjectA:

containerObjectB.variables['whatever'] = Variable.data(); final edited = containerObjectA == containerObjectB;

Problem

Step 2 technically works because I saw that you are making use of DeepEquality. The problem is that it fails because step 1 does not work afaic. I realize that the idea here would be not edit the map and pass an altered map to copyWith. But how am I supposed to do that? Then, I am back to not having any benefit from freezed and it gets worse when using Lists.

I cannot know the Map keys ahead of time and if I had a list, there would be no way to represent it as a freezed value anyway.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rrousselGit/freezed/issues/185, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEZ3I3PSXCBVPL52YSCWYG3RTJVCLANCNFSM4NJQQW6A .

rrousselGit avatar May 25 '20 13:05 rrousselGit

@rrousselGit

As far as I am concerned, freezed only provides deep copies for nested @freezed objects but not for Iterables.
In some cases, you cannot know the number of items you have or the map keys you will receive and then you need to use List or Map. It would be nice if freezed would also deep copy those.

Oh, I thought this was a request for something else.

Yeah, I plan on adding support for collections at some point. I originally mentioned it on the deep copy issue https://github.com/rrousselGit/freezed/issues/80#issuecomment-593389179

But you can use normal copy in the meantime. Especially with the ... operator

rrousselGit avatar May 25 '20 16:05 rrousselGit

How is the syntax in https://github.com/rrousselGit/freezed/issues/80#issuecomment-593391413 supposed to work? The list is not callable (at least for me).

list.todos([]);

Deep collection copy is not implemented yet. The syntax would be slightly different now.

We'd have:

@freezed
abstract class TodoList with _$TodoList {
  factory TodoList(List<Todo> todos) = _TodoList;
}
@freezed
abstract class Todo with _$Todo {
  factory Todo(String name, bool checked) = _Todo;
} 

Then used this way:

var list = TodoList([Todo('foo', false)]);

list = list.copyWith.todos.add(Todo('bar', false));

var allChecked = list.copyWith.todos.assign(checked: true);

Or something among those lines.

rrousselGit avatar May 25 '20 17:05 rrousselGit

So a syntax like this, will likely be possible:

var list = list.copyWith.todos[0](checked: true);

But is it even possible, that copyWith could be combined with the functions that are callable on normal Lists (insert, singleWhere, sort, ...)

var list = list.copyWith.todos.firstWhere((e) => e.name == 'bar').(checked: true);

I know probably a stupid question, just wanna know what syntax to focus on, when I have time to have a look on this feature.

SunlightBro avatar Jun 02 '20 08:06 SunlightBro

But is it even possible, that copyWith could be combined with the functions that are callable on normal Lists

These aren't functions from the official List

It'd be a reimplementation of Iterable adapted for clone

rrousselGit avatar Jun 02 '20 14:06 rrousselGit

How to handle iterables/maps in current state? i came up with this:

void selectFromAirport(SearchAirport airport, int index) {
  search = search.copyWith(
            routes: search.routes // List<SearchRoutes> routes;
              ..[index].copyWith(
                fromAirport: airport.code,
                fromName: airport.name,
              )
              ..toList(),
          );
}

is it ok to mutate list in this case or should i use immutable list library like https://github.com/passsy/kt.dart?

ksmsk avatar Jun 23 '20 14:06 ksmsk

Don't mutate the list Make a copy instead, with the... operator

[...search.routes]..[index] = newValue;

rrousselGit avatar Jun 23 '20 15:06 rrousselGit

How to return a deep copy of collection _users with 1 room removed? The following code won't work. A Selector will not notice that a collection was changed and will not rebuild widgets.

class UsersStore extends ChangeNotifier {
  Map<int, User> _users = {
    1: User(
      houses: {
        1: House(
          rooms: Room.values.toSet(),
        ),
      },
    ),
  };
  Map<int, User> get users => Map.of(_users);

  void removeRoom(int userId, int houseId, Room room) {
    _users[userId].houses[houseId].rooms.remove(room);    
    notifyListeners();
  }
}

@freezed
abstract class User with _$User {
  factory User({@required Map<int, House> houses}) = _User;
}

@freezed
abstract class House with _$House {
  factory House({@required Set<Room> rooms}) = _House;
}

enum Room { bedroom, kitchen, toilet, hall, boxroom }

~Also ... operator is not suitable for Maps.~

subzero911 avatar Aug 10 '20 11:08 subzero911

@rrousselGit I come up with using kt_dart according to this article: https://levelup.gitconnected.com/flutter-dart-immutable-objects-and-values-5e321c4c654e

Still thinking about proper map element removal though... the method in the article is not correct image

subzero911 avatar Aug 11 '20 12:08 subzero911

I took me a whole lot of time, but I found a solution:

image

subzero911 avatar Aug 12 '20 15:08 subzero911

Here is the problem though. We don't have any null-safety. What if we're trying to access the field that does not exist? Say, if we want to add a room to the house or user which doesn't exist?

context.read<UsersModel>().addOrReplaceRoom(4, 5, Room.toilet),  // throws

To handle all these cases I had to provide a default value for EACH [key] occurence. And it becomes a real mash:

image

subzero911 avatar Aug 12 '20 23:08 subzero911

@subzero911 In this case I think the null operator could be overloaded right? So you'd have something like

users?.houses?.houseId?.rooms

This would be what the package author would have to implement rather than the user. I'm sure this would be easier if it were directly in the package though of course.

satvikpendem avatar Sep 03 '20 12:09 satvikpendem

To handle all these cases I had to provide a default value for EACH [key] occurence. And it becomes a real mash:

That's a mash in all languages, even without immutability/copy. Freezed can improve the situation, but it certainly isn't specific to Freezed.

Anyway, I wonder what the syntax would be in your example for deep copy? Maybe:

_users = {
  ..._users,
  userId: _users[userId] == null
    ? User(houses: {houseId: House(rooms: {room})})
    : _users[userId].copyWith.houses.at(houseId, orElse: () => const House(rooms: [])).rooms.append(room)
};

rrousselGit avatar Sep 03 '20 13:09 rrousselGit

At the end I gave up this venture and moved to MobX which has Observable collections, allowing to write code in a mutable style, and it does not need copyWith at all.

subzero911 avatar Sep 10 '20 17:09 subzero911

Couldn't there be some ImmerJS-type syntax? I know many React devs use it for immutable-but-mutable-looking state changes.

satvikpendem avatar Sep 11 '20 01:09 satvikpendem

@rrousselGit back when I created the issue, I was not even aware of the use case, however, I think that basic list are a way bigger use case! Should we create a new issue to track this because it is not really clear in my original comment? I "mentioned" it, but I did not elaborate on it.

Essentially, imagine a simply freezed object:

@freezed
class GuestList with _$GuestList {
  @JsonSerializable(explicitToJson: true)
  const factory GuestList({
    required List<Guest> guests,
  }) = _GuestList;

  factory GuestList .fromJson(Map<String, dynamic> json) =>
      _$GuestListFromJson(json);
}

Guest is also a freezed object that simply has a name and age property.


Now, copyWith is fully broken / unusable for all use cases I can imagine:

  1. Change the name of one guest.
  2. Remove one guest from the guest list.
  3. Add a guest to the guest list.

The copyWith syntax does not support any of these. We also cannot use List.add, List.remove, and Guest.copyWith as it would also mutate the original object.

The only option is essentially manually doing a deep copy using List.of.

Now, copyWith is fully broken / unusable for all use cases I can imagine:

That's probably an exaggeration. Deep copy is optional. copyWith itself works just fine:

GuestList list;

list = list.copyWith(
  guests: [
    ...list.guests,
    Guest(),
  ]
);

rrousselGit avatar Apr 12 '21 09:04 rrousselGit

That's probably an exaggeration. Deep copy is optional. copyWith itself works just fine

@rrousselGit I understand. The reason I phrased it this way is the fact that in this case copyWith() is the same as GuestList().

Right, but only because your example has a single property / case. If your class does nothing but hold a list, you could remove it and directly use List<Guest> instead of a Freezed class.

But back on the topic, the main reason I haven't implemented such feature is because I'm not quite satisfied with the proposed syntax.

The idea of:

todoList.copyWith.list.add(Todo())
  • lacks flexibility. What if we want to add two items for example? .add().add won't work
  • doesn't read well. .add feels out of place and sound like an operation

rrousselGit avatar Apr 12 '21 12:04 rrousselGit

If your class does nothing but hold a list, you could remove it and directly use List<Guest> instead of a Freezed class.

Not really because of JSON related stuff a Freezed class offers.

The best thing would be having immerjs like functionality on doing copies.

robertpiosik avatar Apr 13 '21 09:04 robertpiosik

Don't mutate the list Make a copy instead, with the... operator

[...search.routes]..[index] = newValue;

My 2 cents:

  1. It would be great to write that in the README.md file to alert people about it (and avoid having strange behaviors and spend some time searching through issues to find why...)

  2. While waiting for this package to offer a way to manage list as immutable, it should be great also to strongly suggest people to use immutable list by using other packages and so get verification at compile time: fast_immutable_collections (which also allow to specify equals identity for each list!) kt_dart built_collection ...

sperochon avatar Nov 19 '21 10:11 sperochon

Hi, I have a question,

final originalBill = Bill(items: [Item()]);
// assume Bill and Item both are freeze class

To add an element to list, I have to

final cloneBill = originalBill.copyWith(items: [...originalBill.items, Item()]));

Since List/Map isn't immutable/freeze, why don't copyWith() clone all List/Map with spread operators?

// so i can write this, without affect originalBill
final cloneBill = originalBill.copyWith();
cloneBill.items.add(Item());
// or this
cloneBill.items = [...cloneBill.items, Item(), Item()];
// i feel more comfortable reading these syntax compare to last one

I am not sure if this is off topic, let me know if I shouldn't ask the question here, thanks in advance.

HJ29 avatar Dec 29 '21 12:12 HJ29

Because that would be very inefficient.
It'd basically mean that Freezed would have to clone all objects all the time, whereas the current implementation clones only what changed.

You're free to do:

final cloneBill = originalBill.copyWith(items: originalBill.items.toList());
cloneBill.items.add(Item());

rrousselGit avatar Dec 29 '21 14:12 rrousselGit

Because that would be very inefficient. It'd basically mean that Freezed would have to clone all objects all the time, whereas the current implementation clones only what changed.

You're free to do:

final cloneBill = originalBill.copyWith(items: originalBill.items.toList());
cloneBill.items.add(Item());

In this example is Freezed regenerating the list by re-adding all the items to a new list meaning its an n operation whenever anything is changed?

grimatoma avatar Jan 10 '22 05:01 grimatoma

Will deep copy covers following example?

@freezed
class RouteState with _$RouteState {
  factory RouteState.home() = HomeState;
  factory RouteState.books() = BooksState;
  factory RouteState.book(int id) = BookState;
}

Following solution is horrible (with different cases for classes with and without properties):

RoutesState deepCopy(RoutesState from) => RoutesState([
      for (var route in from.routers)
        route.map(
          home: (_) => HomeState(),
          books: (_) => BooksState(),
          book: (route) => route.copyWith(),
        )
    ]);


PavelPZ avatar Jan 16 '22 11:01 PavelPZ

After two weeks of working with riverpod and freezed, I must say that this is a perfect synergy. My app model is all fine-tuned now only in the dart project, without writing a single flutter widget.

Thanks so much for those tools and I apologize for somewhat expressive expression ("horrible") in the previous post.

PavelPZ avatar Jan 29 '22 15:01 PavelPZ

I still run into this issue nearly every time I use freezed (which is usually for immutable state classes for redux). Is there any roadmap for getting this sorted out?

I realize it's a complex issue as I've tried to figure out the best way to implement it myself and maybe do a PR. Admittedly, although I was able to solve a couple of my use cases, I was not able to get very far due to what appears to be a significant learning curve to understand the inner workings of freezed. I realized it was hard to conceptualize what was happening across all the abstract classes, implements, and redirected constructors. Things rarely worked the way I expected when I edited them.

I came back here to see if any real work was still being done by anyone who does understand how freezed works under the hood.

point-source avatar Jul 09 '22 18:07 point-source

This is not something that is being worked on.

rrousselGit avatar Jul 10 '22 12:07 rrousselGit