-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add Action0 and Action1 that will be able to be made null-safe #190
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
|
||
/// Like [Action1], but payloads cannot be made non-nullable since the argument | ||
/// to [call] is optional. | ||
class Action<T> extends Action1<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought is that we'd either:
- keep this type around as a common base type, but could change it to an interface
- use Action1 as the common base type where needed
/// Like [Action1], but without an expected payload (aka a void payload). | ||
class Action0 extends Action1<void> { | ||
@override | ||
String get disposableTypeName => 'Action1'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be 'Action0'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one comment and question, otherwise the approach generally looks good to me, and I can't think of any issues or potential issues that aren't also present in the other approach (Action
/Action2
).
@@ -20,11 +20,30 @@ import 'package:w_common/disposable.dart'; | |||
|
|||
import 'package:w_flux/src/constants.dart' show v3Deprecation; | |||
|
|||
/// Like [Action1], but without an expected payload (aka a void payload). | |||
class Action0 extends Action1<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to consider making this extends Action<Null>
instead, to help prevent consumers from accidentally passing in values without knowing they'll be discarded.
Example: https://dartpad.dev/?id=97af2f0dd8c7919807bd6864ab085794
The relevant pieces from that ^
main() {
{
final action = VoidAction0();
action();
action(null);
action('foo');
// ^^^^^
// No error or other indication that there's a potential issue with
// this usage.
//
// The value being passed in is discarded, but the consumer may not know that.
}
{
final action = NullAction0();
action();
action(null);
action('foo');
// ^^^^^
// Error: The argument type 'String' can't be assigned to the parameter type 'Null'.
}
}
class VoidAction0 extends Action1<void> {
@override
Future call([void payload]) => super.call(null);
}
class NullAction0 extends Action1<Null> {
@override
Future call([Null payload]) => super.call(null);
}
if (isOrWillBeDisposed) { | ||
throw StateError('Store of type $runtimeType has been disposed'); | ||
} | ||
@deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason this was deprecated and triggerOnAction1 added, other than cleanliness and improved naming consistency? Those seem like good enough reasons, but I was curious if there was some other factor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope that was it
Talked through this as a team and decided that this still isn't good enough to justify the change/extra work, so we'll stick with #188 |
No description provided.