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

Add Action0 and Action1 that will be able to be made null-safe #190

Closed
wants to merge 1 commit into from

Conversation

evanweible-wf
Copy link
Contributor

No description provided.

@aviary-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

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> {
Copy link
Contributor Author

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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be 'Action0'

Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a 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> {
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope that was it

@evanweible-wf
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants