Improve FluxUiComponent disposed store logging #703
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
| /// List of store subscriptions created when the component mounts. | ||
| /// | ||
| /// These subscriptions are canceled when the component is unmounted. | ||
| List<StreamSubscription> _subscriptions = []; | ||
|
|
||
| void _validateStoreDisposalState(Store store) { | ||
| String message = 'Cannot listen to a disposed/disposing Store.'; | ||
| if (store.isOrWillBeDisposed) { |
There was a problem hiding this comment.
I wrapped both the assert and the log in this check so we could conditionally build a shared message instead of computing it unnecessarily. The control-flow of this method should be the same as it was before.
|
|
||
| var isDisposedOrDisposing = store.isOrWillBeDisposed ?? false; | ||
| final message = 'Cannot listen to a disposed/disposing Store.' | ||
| ' (storeNameOrType: $storeNameOrType, shouldBatchRedraw: $shouldBatchRedraw)'; |
There was a problem hiding this comment.
Would it be better to use a ContextualMessage and put storeNameOrType and shouldBatchRedraw in the context of the message instead?
There was a problem hiding this comment.
I really wanted to do that, but unfortunately I'd have to depend on app_intelligence_dart to get that class (which we can't do since this is an OSS repo, but even if it weren't I'd be wary about).
It'd be nice if you could pass in a Map or some other core type and have it treat it the same as a ContextualMessage.
There was a problem hiding this comment.
Ahh. I didn't realize that was an AID-specific class.
|
QA +1. The unit tests added should be sufficient for this change. |
|
@Workiva/release-management-p |
Motivation
We have some warning logs in place for before FluxUiComponent(2) components attempt to subscribe to disposed stores.
These logs weren't very helpful, though, since they didn't contain a stack trace, component name, or store name.
Changes
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: