- Notifications
You must be signed in to change notification settings - Fork22
Add filterBreadcrumbActions option#39
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-io commentedOct 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #39 +/- ##===================================== Coverage 100% 100% ===================================== Files 1 1 Lines 18 21 +3 Branches 6 8 +2 =====================================+ Hits 18 21 +3
Continue to review full report at Codecov.
|
How about instead of an array of types, we allow the user to supply a Also, I think the action should still be set as the Thanks for the PR, I think this will be a great improvement. |
@captbaritone Good idea. Updated PR with your suggestion. The test I added now confirms that the filtered action was omitted from breadcrumbs, but still included in lastAction. Thanks! |
index.js Outdated
| message:action.type, | ||
| data:breadcrumbDataFromAction(action) | ||
| }); | ||
| if(filterBreadcrumbActions(action)===true){ |
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 think it would be less confusing for this to just check for truthy values.
if (filterBreadcrumbActions(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.
Yeah, I did that at first, but thought being explicit might catch some bugs in the user-supplied function.
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 think truths/falsy is a perfectly valid return value from a filter predicate. Not nessesarily a bug.
| given the category`"redux-action"`. If you would prefer a different category | ||
| name, specify it here. | ||
| ####`filterBreadcrumbActions`_(Function)_ |
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 might also be worth adding to the "improvements" section above.
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 small thing. Thanks for the quick turn around.
| extra, | ||
| breadcrumbs | ||
| }=context.mockTransport.mock.calls[0][0].data; | ||
| // Even though the action isn't added to breadcrumbs, it should be sent with extra data |
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 think this would be better as a separateit() block, that way if it fails, we get a nice error message, but don't worry about it. I can always clean this up later.
Looks good! I'll cut a point release. |
Thanks for the quick feedback & merge! This is definitely going to improve our debugging process. |
Okay. Should be available in v1.1.0 Thanks again for your work on this! |
Uh oh!
There was an error while loading.Please reload this page.
filterBreadcrumbActions is a function that returns
trueif an action should be added to the breadcrumbs.The rationale for this is that some Redux apps use some actions internally to trigger state changes that are both prolific and not directly connected to user behavior. They can overwhelm the logs and have essentially no value for debugging purposes.