brianegan / flutter_redux

A library that connects Widgets to a Redux Store
MIT License
1.65k stars 219 forks source link

[Question] Why is the default value of `distinct` set to `false`? #200

Closed eXamadeus closed 3 years ago

eXamadeus commented 3 years ago

Preface

I am pretty new to Flutter and Dart, just poking around at it. I am; however, very experienced with Redux in React. My question may just be simply because I don't understand the ecosystem, flutter, dart, something. Anyway, please forgive me if this is a "dumb question".

Why is the default value of distinct set to false?

I come from the React/TypeScript world, so I'm quite familiar with Redux as a state management tool. Redux behaves in a pleasantly simple way and that's no exception here in Flutter. There is a store that lives at the top of the React component tree (App) and consumers of that store are components (Widgets) that live below that store and "connect" to it. Seems to be pretty much the same in Flutter.

The latest iteration of this "connection pattern", in React, manifests as the useSelector hook (for selecting data, dispatches are different, but those are irrelevant here, I think). This hook performs the task of "connecting" the component to the store and "subscribes" it to updates. It does this rather opinionated manner by only forcing the component to re-render if the selected value is different from the previous result. This means components only re-render when the data they "select" changes or they get a props/state change from somewhere else (not important here).

The proper analogy to the useSelector hook in Flutter would be the StoreConnector, right? In that case, instead of doing something like this in React:

const Foo = () => {
    const count = useSelector(state => state.count);

    return <span>The count is {count}</span>;
};

we would model it like this in Flutter (types removed just to make things easier to quickly read):

class FooContainer extends StatelessWidget {
  @override
  build(_) {
    return StoreConnector(
      distinct: true,
      converter: (store) => store.state.count,
      builder: (_, count) {
        return Text(count);
      },
    );
  }
}

I would say the Flutter example models the pre-hooks style of using connect quite well. Because of that, I find it very easy to reason with.

The thing that strikes me as odd is the necessity of the distinct value. In almost any case I can think of, I only ever want to rebuild a Widget if the state I am "viewing" changes. If something else changes...well I just don't care right? So why does this library buck convention and default to automatic rebuilding when any value in the store changes?

If my understanding of this is right, why even have the option at all? I would argue it's better to remove the distinct value and only update Widgets when their ViewModel changes. I am just having a hard time figuring out why that would be beneficial under any circumstance.

Maybe I am misunderstanding something fundamental here. Again, I am a Flutter noob, so go easy on me.

brianegan commented 3 years ago

Hey mate -- sorry, busy times. No problem at all, it's a totally fair question :) May I start with some trivia to see if this helps explain my reasoning?

If you used the following class as your ViewModel, what would you expect to happen if you compare two objects?

class MyViewModel {
  final int id;
  final String name;

  MyViewModel(this.id, this.name); 
}
  1. MyViewModel(1, 'Brian') == MyViewModel(1, 'Brian')
  2. MyViewModel(1, 'Brian') != MyViewModel(1, 'Brian')

Or, let's make it even more fun, which of the following statements is true using the following class :)

class MyViewModel {
  final String name;
  final void Function() onNameTapped;

  User(this.name, this.onNameTapped);

  @override
  bool operator ==(Object other) =>
      identical(this, other) ||
      other is User &&
          runtimeType == other.runtimeType &&
          name == other.name &&
          onNameTapped == other.onNameTapped;
}
  1. MyViewModel('Brian', () { print('yo'); }) == MyViewModel('Brian', () { print('yo'); })
  2. MyViewModel('Brian', () { print('yo'); }) != MyViewModel('Brian', () { print('yo'); })

In both of these cases, the objects are NOT equal. In the first case, you might be clever and noticed I'm not implementing ==, so the two objects will be compared by identity. Since they are two separate instances, they are NOT equal to one another.

In the second case, you might think "Surely they must be. They're overriding the == operator this time!" Nope. "brian" == "brian" is fine, but () { print('yo'); } != () { print('yo'); } is not true!

What's the point I'm making? In order for distinct: true to do anything, you need to properly implement the == operator and understand a few gotchas that come along with it. That can be a bit tricky. Therefore, I made it false by default so folks wouldn't think they're getting a free performance optimization.

I don't know if that was the right call, but I've seen those two exact issues trip up enough people that it felt deceptive to leave distinct: true as the default.

What do you think of that reasoning?

eXamadeus commented 3 years ago

@brianegan Thanks for the thoughtful and thorough answer!


My reply and thoughts

I see where you're going, and you're basically saying "shallow comparisons are confusing". Funny enough, that's actually exactly how it works in the React world where Redux came from (which I'm sure you already knew) but they just "let people deal with it" haha.

In order for distinct: true to do anything, you need to properly implement the == operator

That's only true if the selected data is non-primitive, right? IE in my example count works because it's an int right (I just remembered I removed the types in my example, but shallow int comparison would work right?)

In React, it's common knowledge that objects, arrays, and functions are non-equivalent during a shallow comparison. Components and selectors still do shallow comparisons by default though, because why not? It's not going to hurt anything by allowing them to be slightly more optimized. The documentation just lets you know: "Hey! Only shallow comparisons are done here." and then people are left to do what they want with that knowledge.

What's funny is JS/TS can't ever do object comparisons, but Dart totally can, if users implement the proper equality checks.

Therefore, I made it false by default so folks wouldn't think they're getting a free performance optimization.

I don't know if that was the right call, but I've seen those two exact issues trip up enough people that it felt deceptive to leave distinct: true as the default.

I completely get where you were coming from and what problem you are trying to solve, but the attempt to help people might actually be a disservice to others who "know what they are doing".


My suggestion: configurable default

What if you make the default value for distinct configurable somewhere during the creation of the store (or maybe the provider)? It could be an option like defaultDistinct that gets passed along when building out the StoreConnector's.

This way you have the best of both worlds. You can leave the default value as false, so it protects people that aren't aware. But people who are cautious can switch the default to true and skip typing out distinct: true a bunch.

I haven't looked at the store code, so I don't know if that would be hard to implement or not. I'm also not sure if that should be a store or provider configuration value, but I think you get my idea. What would you think of that?

I love the idea of making people aware though, so I'm not suggesting removing distinct anymore. In fact, I really like having that control. But it would be nice to take it a step further and decide the default on the fly.

brianegan commented 3 years ago

I completely get where you were coming from and what problem you are trying to solve, but the attempt to help people might actually be a disservice to others who "know what they are doing".

My suggestion: configurable default

Perhaps this is just me, but I actually have a similar requirement quite often when it comes to Flutter's default settings.

For example, maybe I want to use a TextField with some configuration options that are different from the Flutter default. In that case, I create some kind of MyTextField wrapper widget which sets the defaults to what I want, then I use that Widget throughout my app in place of the regular TextField.

What do you think of that option?

eXamadeus commented 3 years ago

Ahh, I think I see what you mean. That does seem like a feasible way to do it. And I think it would certainly solve this use case.

I still think I'd be in the camp of "rather configure it once at the root and not maintain custom wrappers" but that's just like...my opinion 😆. Also, I think that it would add a lot of complexity for a preference. So I totally get the reasoning behind "leaving things as they are".

All in all, thanks for the discussion. I think we can mark this as "answered"!

hacker1024 commented 3 years ago

Would it be possible to use an analyzer plugin to add Dart lints to help ensure the distinct parameter isn't forgotten? The lint could check if a used ViewModel has == implemented, and display a warning if distinct is left unset.

Alternatively, it could work the other way round, and warn if distinct is used when == isn't implemented. This would allow distinct to default to true, avoiding the "free performance optimisation" illusion.

This may have to wait until https://github.com/dart-lang/linter/issues/697 is resolved.

whuhewei commented 1 year ago

If set distinct true and not implement ==, then It still works(UI won't update) when the connected ViewModel itself doesn't change. The above 'change' means your action doesn't update the connected ViewModel. In conclusion, I vote for set distinct value default true.