refactor: migrate widget side effects over to use.disposable
#235
Conversation
|
|
||
| controller | ||
| (controller) => controller.dispose(), | ||
| ) | ||
| ..duration = duration | ||
| ..reverseDuration = reverseDuration; |
There was a problem hiding this comment.
I was wondering to rewrite it this way:
AnimationController _animationController(
WidgetSideEffectRegistrar use, {
Duration? duration,
Duration? reverseDuration,
String? debugLabel,
double initialValue = 0,
double lowerBound = 0,
double upperBound = 1,
TickerProvider? vsync,
AnimationBehavior animationBehavior = AnimationBehavior.normal,
}) {
vsync ??= use.singleTickerProvider();
return use.disposable(
() => AnimationController(
vsync: vsync!,
duration: duration,
reverseDuration: reverseDuration,
debugLabel: debugLabel,
lowerBound: lowerBound,
upperBound: upperBound,
animationBehavior: animationBehavior,
value: initialValue,
),
(controller) => controller.dispose(),
[duration, reverseDuration, debugLabel, initialValue, lowerBound, upperBound, vsync, animationBehavior]
);
}I've seen examples of passing the arguments as dependencies. But in this case, I thought there might be a reason not to pass them as deps. Am I right?
There was a problem hiding this comment.
I think i made the decision that it should keep the same controller instance here regardless of whether those params change, which is why I have that string of ..s for duration and reverseDuration at the end.
Normally, you should put any captured variables in the dependencies list, but this case is an exception since we don't want to return a new controller (at least I don't think that would be the desired behavior).
|
|
||
| controller | ||
| (controller) => controller.dispose(), | ||
| ) | ||
| ..duration = duration | ||
| ..reverseDuration = reverseDuration; |
There was a problem hiding this comment.
I think i made the decision that it should keep the same controller instance here regardless of whether those params change, which is why I have that string of ..s for duration and reverseDuration at the end.
Normally, you should put any captured variables in the dependencies list, but this case is an exception since we don't want to return a new controller (at least I don't think that would be the desired behavior).
| return use.disposable( | ||
| () => TextEditingController(text: initialText), | ||
| (controller) => controller.dispose(), | ||
| [initialText], |
There was a problem hiding this comment.
The controller should not be recreated if initialText changes, so would you mind removing the [initialText], line?
GregoryConrad
changed the title
use.disposable
No description provided.