Add option to clear action logger #3459

Keraito · 2018-04-20T20:46:29Z

Issue: Currently, the action loggers stacks up all the events without limit and across all stories. There is already a PR (#3447) open for a limiting option. In the same way, this introduces an optional feature that will clear the action logger when switching away from a story. (Fixes #3443)

What I did

Added a clearActionLogger option to the actions addon that clears the action logger when switching away from a story. It uses the storybook/stories/story-event event for detecting a switch in story.

How to test

I updated the configureAction unit test and the documentation on action options.

Keraito · 2018-04-20T20:54:00Z

In the corresponding issue @merlinpatt mentioned that he would be willing to do this. I just decided to start working on it and proposed my current progress. If he wants to take over or maybe work together I dont mind.

Hypnosphi · 2018-04-20T20:59:07Z

Please use this option in examples/storybook-official

codecov · 2018-04-20T21:13:35Z

Codecov Report

Merging #3459 into master will decrease coverage by <.01%.
The diff coverage is 12.5%.

@@            Coverage Diff             @@
##           master    #3459      +/-   ##
==========================================
- Coverage   37.14%   37.13%   -0.01%     
==========================================
  Files         465      465              
  Lines       10320    10328       +8     
  Branches      924      949      +25     
==========================================
+ Hits         3833     3835       +2     
+ Misses       5933     5906      -27     
- Partials      554      587      +33

Impacted Files	Coverage Δ
...ddons/actions/src/containers/ActionLogger/index.js	`0% <0%> (ø)`	⬆️
addons/actions/src/manager.js	`0% <0%> (ø)`	⬆️
addons/actions/src/preview/action.js	`77.41% <100%> (+0.75%)`	⬆️
addons/actions/src/preview/configureActions.js	`100% <100%> (ø)`	⬆️
lib/cli/lib/has_yarn.js	`0% <0%> (ø)`	⬆️
addons/storysource/src/loader/parse-helpers.js	`46.75% <0%> (ø)`	⬆️
addons/events/src/components/Event.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/types/proptypes.js	`88.88% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Button.js	`11.9% <0%> (ø)`	⬆️
... and 79 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34e8e29...36ff731. Read the comment docs.

Keraito · 2018-04-21T13:35:12Z

Something like this, @Hypnosphi?

Hypnosphi · 2018-04-21T14:00:43Z

examples/official-storybook/stories/addon-actions.stories.js

+  })
+  .add('Clearing the action logger', () => (
+    <div>
+      <p>Moving away from this story will clear the action logger</p>


I think it would be more natural if it worked the other way around, and logger would be cleared when opening this story. Can we do this?

I think a natural way for this to work would indeed be:

When the user navigates between stories, it's cleared by default,
unless a user opts to have the log be persisted.

I at first tried to do it the way @Hypnosphi described it, but it's slightly more difficult that way because the options are part of the actions and not the stories themselves. The way @ndelangen sounds more natural and should be of less complexity, so I will try my hands on that.

merlinstardust · 2018-04-21T21:55:36Z

Sorry I'm late to the conversation but isn't there an overall Storybook configuration that this could go in?

Hypnosphi · 2018-04-21T22:01:19Z

@merlinpatt So far, not really. Each addon exposes its own configuration function when needed

igor-dv · 2018-04-22T13:09:47Z

addons/actions/README.md

@@ -102,3 +102,4 @@ action('my-action', {
 |Name|Type|Description|Default|
 |---|---|---|---|
 |`depth`|Number|Configures the transfered depth of any logged objects.|`10`|
+|`clearActionLogger`|Boolean|Flag whether to clear the action logger when switching away from the current story.|`false`|


I would call it "clearOnStoryChange"

merlinstardust · 2018-04-22T13:18:39Z

Oh right, I forgot because the action logger is part of the default Storybook setup.

Does the action logger have some sort of top level configuration?

While I see the value in having a user decide to clear it per story, I also find it likely that users would want to clear it on every story change. And that would be frustrating to have to add an option to every single story

Keraito · 2018-04-22T14:03:01Z

Does the action logger have some sort of top level configuration?

@merlinpatt check out this on configureActions, I think that's what you're looking for.

I also find it likely that users would want to clear it on every story change

Based on the other comments, I will be changing the default behaviour to this

Keraito · 2018-04-22T19:01:19Z

I processed both feedback of #3459 (comment) and #3459 (comment).

Behaviour wise, actions are still only persisted when switching away from a story. While changing the default to true gets rid of the counter-inituitive behaviour described in #3459 (comment), I didn't see a way yet to make this happen on entry.

Hypnosphi · 2018-04-22T19:07:26Z

actions are still only persisted when switching away from a story

That's OK, you're kinda saying "I want logs of this story to be persisted"

rhalff

The event id the story change listener depends on (storybook/stories/story-event) seems to be triggered by the storysource addon: https://github.com/storybooks/storybook/blob/master/addons/storysource/src/index.js#L1-L3

Unless the storybook api is changed in the near future anyway, I wouldn't rely on this particular event.

Otherwise at least import the event id as import { EVENT_ID } from '@storybook/addon-storysource so it's clear where the event originates from.

Hypnosphi · 2018-04-22T20:58:55Z

I think this one should be emitted by core (and imported from there)

@rhalff thanks for noticing that

rhalff · 2018-04-22T21:35:32Z

Currently there is an onStory() method defined on the api which could be used, it is passed in during register of the addon.

Usage of it is a bit awkward though, e.g.:

export function register() {
  addons.register(ADDON_ID, (api) => {
    const channel = addons.getChannel();
    
    addons.addPanel(PANEL_ID, {
      title: 'Action Logger',
      render: () => <ActionLogger channel={channel} api={api} />,
    });
  });
}

// do something with api.onStory in the ActionLogger
api.onStory((kind, story) => ....)

Keraito · 2018-04-22T21:48:08Z

Thank you @rhalff, I will look into the onStory method and propose changes accordingly

Keraito · 2018-04-23T17:43:58Z

Like this @rhalff?

Keraito · 2018-04-23T20:37:15Z

Added it, thanks for the info @rhalff.

Keraito added 3 commits April 20, 2018 17:17

Add the option to clear action logger

7c02698

Fix action configuration test

3547919

Update configuration descriptions

ae2292f

Keraito requested a review from rhalff as a code owner April 20, 2018 20:46

Hypnosphi added feature request addon: actions labels Apr 20, 2018

Add actionlogger clear example

0d452b1

Keraito requested review from danielduan, Hypnosphi and usulpro as code owners April 21, 2018 13:34

Hypnosphi reviewed Apr 21, 2018

View reviewed changes

igor-dv reviewed Apr 22, 2018

View reviewed changes

Keraito added 2 commits April 22, 2018 20:55

Change default and flag name

892f675

Update the example and test

51ee317

Hypnosphi approved these changes Apr 22, 2018

View reviewed changes

rhalff suggested changes Apr 22, 2018

View reviewed changes

Change event listened on to api onStory

0b50364

Adjust PropTypes

a44bace

Properly remove the on story listener

36ff731

rhalff approved these changes Apr 23, 2018

View reviewed changes

Hypnosphi merged commit a735eeb into storybookjs:master Apr 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to clear action logger #3459

Add option to clear action logger #3459

Keraito commented Apr 20, 2018

Keraito commented Apr 20, 2018

Hypnosphi commented Apr 20, 2018

codecov bot commented Apr 20, 2018 •

edited

Loading

Keraito commented Apr 21, 2018

Hypnosphi Apr 21, 2018

ndelangen Apr 21, 2018 •

edited

Loading

Keraito Apr 21, 2018 •

edited

Loading

merlinstardust commented Apr 21, 2018

Hypnosphi commented Apr 21, 2018

igor-dv Apr 22, 2018

merlinstardust commented Apr 22, 2018

Keraito commented Apr 22, 2018 •

edited

Loading

Keraito commented Apr 22, 2018

Hypnosphi commented Apr 22, 2018

rhalff left a comment •

edited

Loading

Hypnosphi commented Apr 22, 2018 •

edited

Loading

rhalff commented Apr 22, 2018

Keraito commented Apr 22, 2018

Keraito commented Apr 23, 2018 •

edited

Loading

Keraito commented Apr 23, 2018

Add option to clear action logger #3459

Add option to clear action logger #3459

Conversation

Keraito commented Apr 20, 2018

What I did

How to test

Keraito commented Apr 20, 2018

Hypnosphi commented Apr 20, 2018

codecov bot commented Apr 20, 2018 • edited Loading

Codecov Report

Keraito commented Apr 21, 2018

Hypnosphi Apr 21, 2018

Choose a reason for hiding this comment

ndelangen Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Keraito Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

merlinstardust commented Apr 21, 2018

Hypnosphi commented Apr 21, 2018

igor-dv Apr 22, 2018

Choose a reason for hiding this comment

merlinstardust commented Apr 22, 2018

Keraito commented Apr 22, 2018 • edited Loading

Keraito commented Apr 22, 2018

Hypnosphi commented Apr 22, 2018

rhalff left a comment • edited Loading

Choose a reason for hiding this comment

Hypnosphi commented Apr 22, 2018 • edited Loading

rhalff commented Apr 22, 2018

Keraito commented Apr 22, 2018

Keraito commented Apr 23, 2018 • edited Loading

Keraito commented Apr 23, 2018

codecov bot commented Apr 20, 2018 •

edited

Loading

ndelangen Apr 21, 2018 •

edited

Loading

Keraito Apr 21, 2018 •

edited

Loading

Keraito commented Apr 22, 2018 •

edited

Loading

rhalff left a comment •

edited

Loading

Hypnosphi commented Apr 22, 2018 •

edited

Loading

Keraito commented Apr 23, 2018 •

edited

Loading