-
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
FEA-2771 Add ActionV2 w/o null safety #188
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
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 some nits, otherwise LGTM. I'm also still working on a spike with Action0 and Action1 (for arity 0 and arity 1 since we talked about that alternative - we can compare them and see what we think.
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.
I've just got some nits around the tests.
@@ -153,7 +153,7 @@ class Store extends Stream<Store> with Disposable { | |||
/// called until that future has resolved. | |||
/// | |||
/// If the `Store` has been disposed, this method throws a [StateError]. | |||
void triggerOnActionV2<T>(Action<T> action, | |||
void triggerOnActionV2<T>(ActionV2<T> action, |
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.
😆 this method name now works on two different levels
4c474cb
QA +1
|
@Workiva/release-management-p ready to merge |
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.
+1 from RM
Motivation
Adding APIs in major versions introduces risk, since consumers with opened ranges could accidentally consume new APIs. So instead of exposing Action2 in w_flux 3.0, we will backpatch Action2 to a w_flux 2.x minor, so that it won't be an API addition in w_flux 3.0. This option enables us to start work on the Action migration before the w_flux major release has finished rolling out
Changes
Add Action2 and appropriate test coverage without null safety work.
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: