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

[0.6.0-dev] useProvider triggers pointless rebuild when the value type is collection #69

Closed
mono0926 opened this issue Aug 3, 2020 · 9 comments
Labels
bug Something isn't working

Comments

@mono0926
Copy link
Sponsor Contributor

mono0926 commented Aug 3, 2020

Describe the bug

useProvider triggers pointless rebuild when the value is collection type.

To Reproduce

At 0.6.0-dev+2, executed this.
build log will be printed each time PRESS ME button is pressed.

import 'package:flutter/material.dart';
import 'package:flutter_hooks/flutter_hooks.dart';
import 'package:hooks_riverpod/hooks_riverpod.dart';

void main() => runApp(const ProviderScope(child: App()));

class App extends StatelessWidget {
  const App({Key key}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: HomePage(),
    );
  }
}

class HomePage extends HookWidget {
  const HomePage({Key key}) : super(key: key);
  @override
  Widget build(BuildContext context) {
    print('build');
    useProvider(_stateProvider.select((value) => value.state));
    return Scaffold(
      appBar: AppBar(),
      body: RaisedButton(
        child: const Text('PRESS ME'),
        onPressed: () => context.read(_stateProvider).state = [42],
      ),
    );
  }
}

final _stateProvider = StateProvider((_) => [42]);

If [42](List<int>) is changed to 42(int), the problem doesn't occurs, so collection comparison seems broken.

Expected behavior

build should be printed at first time when the UI appeared, and shouldn't be printed when PRESS ME button pressed.
At 0.5.1, it works well without any problems.

@mono0926 mono0926 added the bug Something isn't working label Aug 3, 2020
@mono0926
Copy link
Sponsor Contributor Author

mono0926 commented Aug 3, 2020

@rrousselGit
Copy link
Owner

This was removed voluntarily

Collection comparison are quite expensive, so it is usually inefficient

@mono0926
Copy link
Sponsor Contributor Author

mono0926 commented Aug 3, 2020

Is there any way to compare collection like previously?

I sometimes prefer this kind of optimization.

class _ListView extends HookWidget {
  const _ListView({ Key key }): super(key: key);
  @override
  Widget build(BuildContext context) {
    final ids = useProvider(someProvider.state.select((s) => s.keys.toList()));
    return ListView.builder(
      itemCount: ids.length,
      itemBuilder: (context, index) => _Tile(id: ids[index]),
    );
  }
}

But I know this kind of optimization is not much needed, so it is okay if collection comparison is impossible.

@rrousselGit
Copy link
Owner

There's a different solution:

final someLengthProvider = Provider((ref) {
  return ref.watch(someProvider).keys.length;
});

return ListView.builder(
  itemCount: useProvider(someLengthProvider), 
);

@mono0926
Copy link
Sponsor Contributor Author

mono0926 commented Aug 3, 2020

Thanks, but the detection of ids's reorder is needed 🤔

@rrousselGit
Copy link
Owner

You could have :

itemBuilder: (context, index) {
  return HookBuilder(builder: (context) {
    final id = useProvider(someProvider.select((s) => s.keys.elementAt(index)) 
    return TodoItem(id);
  });
})

@mono0926
Copy link
Sponsor Contributor Author

mono0926 commented Aug 3, 2020

Thanks, it suppresses the pointless rebuild and works well 👍

https://github.com/mono0926/riverpod_example/blob/5a69ef38ffbf5e9654821d4727da52374c1b7217/lib/pages/counters_page/counters_page.dart#L21-L35

I think this change should be documented(although you may already be planning on it).

The problem was fixed, so I'm closing the issue, thanks again!

@mono0926 mono0926 closed this as completed Aug 3, 2020
@rrousselGit
Copy link
Owner

Such code is how Todo and Marvel deal with lists, with using ScopedProvider on the top of that to make the list item constant

@mono0926
Copy link
Sponsor Contributor Author

mono0926 commented Aug 3, 2020

Thanks, that's much better 👍
Indeed I was concerned the rebuild of each item rather than ListView itself.
So, I reverted the change, and made _Tile as const.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants