Test: Create @storybook/test package based on vitest #24392
Conversation
…ill use it for the dts files, as bundling dts files is buggy
|
@SocketSecurity ignore-all |
kasperpeulen
added
ci:merged
kasperpeulen
force-pushed
the
kapser/test-package2
branch
from
000505a to
4f08e77
Compare
|
\o/ Nice work, can't wait to try it out. I didn't review very closely, but one thing I did notice is that this new package does not have a README, should it? Might be nice for those visiting its NPM page, and sometimes pages in Storybook docs link to package READMEs, I think. |
There was a problem hiding this comment.
Hey, nice job with the PR, useful stuff. One problem I found (not sure if this code is the cause, but it's the closest that I was able to find) is that now not only the fn() arg types are logged in the Actions tab, but all the fn()s defined anywhere. Not sure if this is expected or a bug, but looking at the code here, that's not how it was meant to work 🧐
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). 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 no 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.
Does that behaviour sound familiar? Anything that can be bypassed / fixed?
There was a problem hiding this comment.
Could you make an issue for this? I have some plans to bypass this behavior, but nothing concrete yet.
There was a problem hiding this comment.
Maybe the easiest thing we can do as a starter, is only log it if the mockName is set:
https://vitest.dev/api/mock.html#mockname
As otherwise it will only log spy and nothing something useful anyway.
I'm also thinking about enriching the fn with a type.
fn().type('action')
fn().type('query')
And only show the type action by default. And hide all other types (or those without type), but still be able to show them by changing the action filter or something.
There was a problem hiding this comment.
Here it is: #27534
Wasn't sure what type of issue that should be; went with bug since seems to me that both the documentation and the code indicate that only the fn()s used in args should be logged in the Actions tab.
There was a problem hiding this comment.
Answering your suggestions @kasperpeulen - could you point out how/where it happens that all fn()s get intercepted and logged in the tab? Looking at the code here, I'd say it's clear that only the ones defined in args should be enhanced 🧐
That knowledge would help me provide better feedback.
There was a problem hiding this comment.
Thanks! I added a link to the code in the response of your issue!
|
@kasperpeulen @yannbf I noticed these lines of code and just wondering if this was a mistake or if this package is intentionally added. https://github.com/storybookjs/storybook/pull/24392/files#diff-a2d5cc6fcc121979458ce4b3a0f89b60993f76e67bf0a54fbe6c970368a278d5R53 It looks like perhaps it got accidentally added? |
|
@stramel I think there was a reason back then, I believe something to do to get vitest playing in the browser. But I can not remember exactly. However, I would love to get rid of the package, as it is quite big. Maybe the recent version of vitest does not need it anymore. |
|
@kasperpeulen Thanks for the reply. That is what I was thinking for why it was included. It looks like The Vitest Browser Mode is experimental still but seems to have most of it working. Would there be interest in a PR to swap Storybook Test over to the newer version? NOTE: It appears that Vitest is using |
Closes #24171
What I did
See the issue
How to test
I added some more unit tests, but this needs some more tests before being merged.
Checklist
MIGRATION.MD
Maintainers
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]🦋 Canary release
This pull request has been released as version
0.0.0-pr-24392-sha-bcf87ea5. Install it by pinning all your Storybook dependencies to that version.More information
0.0.0-pr-24392-sha-bcf87ea5kapser/test-package2bcf87ea51698396399)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=24392