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

[Bug]: All fn() stubs get logged in the Actions panel, not just the component's args #27534

Closed
jalooc opened this issue Jun 4, 2024 · 5 comments · Fixed by #28091
Closed

Comments

@jalooc
Copy link

jalooc commented Jun 4, 2024

Describe the bug

Not only the fn() arg types are logged in the Actions tab, but all the fn()s used anywhere.

A sample case I've been tackling with, after upgrading SB to v8, is that I've got quite a lot of stubs defined using fn() for mocked api modules in my application (using the __mock__ folder method with storybook-addon-manual-mocks). All their invocations get logged in the Actions panel when the component story is invoked, but it does not make sense to have them logged in there - they're neither actions, nor anything relevant to the person using the story. In some cases, when a more complex component, like a page component, makes lots of those mocked api requests, this behaviour renders the Actions tab completely unusable, littering it with irrelevant entries.


First mention: #24392 (comment)

Reproduction link

https://stackblitz.com/edit/github-xdgoog?file=src%2Fstories%2F__mocks__%2Fapi.ts

Reproduction steps

  1. Go to any Page story.
  2. See an item logged in the Actions tab.
    image
  3. Note that there are no action arguments passed to the story - it's just the fn() stub from the __mocks__ directory being logged.

System

Storybook Environment Info:

  System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm <----- active
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    @storybook/addon-essentials: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/addon-interactions: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/addon-links: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/addon-onboarding: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/blocks: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/react: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/react-vite: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    @storybook/test: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    storybook: ^8.2.0-alpha.5 => 8.2.0-alpha.5 
    storybook-addon-manual-mocks: ^1.0.4 => 1.0.4

Additional context

No response

@shilman
Copy link
Member

shilman commented Jun 5, 2024

@kasperpeulen is there any way to disable logging of spies if it's not desired?

@kasperpeulen
Copy link
Contributor

@jalooc We do want to (allow people to) show spies in the action panel:
https://x.com/KasperPeulen/status/1777934882900963702

We use it extensively in our RSC demo. It show mocks that we automatically set for the nextjs framework (cookies/redirect etc.) And it also shows the spies that are manually attached to the (in memory) database.

https://github.com/storybookjs/storybook-rsc-demo
https://66228a3205a9e64e345243d6-ylfrfcbkzm.chromatic.com/?path=/story/app-note-id-page--logout-should-delete-cookie
image

But I do agree with you, it very fast become way too noisy. Actually we hardcoded to not show all spies in the action panel that the nextjs framework adds by default:

const onMockCall = global.__STORYBOOK_TEST_ON_MOCK_CALL__ as typeof onMockCallType;
onMockCall((mock, args) => {
const name = mock.getMockName();
// TODO: Make this a configurable API in 8.2
if (
!/^next\/.*::/.test(name) ||
[
'next/router::useRouter()',
'next/navigation::useRouter()',
'next/navigation::redirect',
'next/cache::',
'next/headers::cookies().set',
'next/headers::cookies().delete',
'next/headers::headers().set',
'next/headers::headers().delete',
].some((prefix) => name.startsWith(prefix))
) {
action(name)(args);
}
});
subscribed = true;
}
};

Which is certainly not ideal, but we didn't have time to make a dedicated action filtering solution yet.

For the short term I propose that we only log spies if they have a name.
Let me know if this fixes your concern @jalooc

You can set a name with fn().mockName('api.users.create'). If it has no name, it hardly adds any value, to see it in the action panel.

For a long term solution, I think it would be best to have 2 mock types:

  • query, this would be for mocking functions that don't mutate, but only query data, typically a GET request
  • action, this would be for mocking that functions that do mutate (might also get some data), typically a POST/PUT/DELETE request

The API might be: fn().type('action') and fn().type('query')

We could automatically enhance all event handlers (onClick etc) to be actions. And then the action panel might only show actions by default, but optionally can show more with some extra UI cc @cdedreuille. Maybe something similar as the chrome console:

image

There still might be cases where you do want to add a name to the spy, and it really is an action, but you still don't want to show it in the action panel by default. For those cases, I think we could have some parameter convention:

  parameters: {
    actions: {
      filter: {
        'API.getAll': false,
        'next/headers::cookies().get': false,
        `^next/router::useRouter\\(\\)`: true // maybe even regex?
      }
    },

Where every addon can disable showing some spies. E.g. this is what the next addon would use. To only show the useful stuff in the action panel.

And then users can:

  • renable spies that are hidden by addons
  • disable some of their own spies to show in the action panel
  • even disable some spies for specific stories with the parameters inheritance

@shilman
Copy link
Member

shilman commented Jun 6, 2024

@kasperpeulen I like your "no name" solution as a short term fix. Let's do it. For the larger fix, please write a short RFC and let's discuss as a small batch project.

@kasperpeulen
Copy link
Contributor

@shilman Made a PR for that #28091

@github-project-automation github-project-automation bot moved this from In Progress to Done in Core Team Projects Jun 6, 2024
@jalooc
Copy link
Author

jalooc commented Jun 20, 2024

Sorry for the late reply. Answering that comment:

Yes, logging spies if they have a name sounds like a reasonable short-term fix.

As for the long-terms solution - not sure about adding the division between query and action, cause it kind of enofrces a quite opinionated view on how a dev should treat the actions, whereas I really liked the freedom of the old actions() api - it allowed me to log in the Actions panel whatever I reckoned made sense for particular story, without limiting myself to the the semantics like query / action.

On that note - wouldn't it be possible to bring back that action() api? E.g. the action function imported from @storybook/test could be the simple fn() under the hood, just transparently enriched with the code logging it in the Actions panel; and the fn() itself could be left untouched in order not to leave devs surprised about any additional behaviour. Choosing to use vitest as a first-class-citizen was imho a great move, but when a dev learns that parts of the vitest's api are modified (have some side effects) and you can't simply rely on it to work the way you learned it from vitest docs, it kind of makes you loose trust in the presumptive behaviour of the api, making the overal experience much less smooth.

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

Successfully merging a pull request may close this issue.

3 participants