-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
CLI: Add addon-console automigration #32083
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
Conversation
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.
Reviewing changes made in this pull request
|
View your CI Pipeline Execution ↗ for commit 8077930
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
cf558cc to
ca186e8
Compare
|
Idea: Write an FAQ entry in the module mocking/spies page about spying and showing actions in the panel. Simple case vs advanced |
86363df to
f6d27c1
Compare
|
For the record, both spies and fn on args work with the HTML framework. No need to add a warning in the doc. |
|
storybook-eol/storybook-addon-console#87 was opened to go alongside this. Remaining tasks require specific permissions on NPM and GitHub and will need to be performed by the core team. |
jonniebigodes
left a comment
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.
@Sidnioulz thanks for taking the time and adjust the documentation as part of this pull request. Appreciate it 🙏 ! I left some items for you to look into when you can and we'll go from there.
Also, while we're at it, can you check the documentation again and fix the spacing and formatting as I noticed some inconsistencies in it.
| <Video src="../_assets/essentials/addon-actions-demo-optimized.mp4" /> | ||
|
|
||
| ## Action args | ||
| ## Story args |
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.
@Sidnioulz, this can be done on a follow-up pull request when the documentation is published. But I just wanted to let you know that since you've renamed this heading, the code (including the example stories) may have links to this documentation. Can you check and see if we're not accidentally breaking any links?
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.
Oof, good point! Is it enough to grep the old URL on the web and storybook repos, or should I look elsewhere / search for more patterns?
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.
To the best of my recollection, the web (documentation site) doesn't reference this. But to be sure, we should check the monorepo and the satellite repositories (including the test-runner) to ensure we're not breaking anything. I'm not that particularly concerned about the EOL ones.
To help you out, I'll check on any private repos that may exist and you don't have access to to verify this as well.
Sounds like a plan?
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.
That was a really good call. Plenty of references in the templates and sandboxes. Using GitHub search:
- Added a commit to this branch
- Opened docs: Update links to actions docs page sandboxes#14
- Opened docs: Update links to actions docs page react-native#785
- Opened docs: Update links to actions docs page addon-react-native-server#12
- Opened docs: Update links to actions docs page vite-plugin-storybook-nextjs#55
- Opened https://github.com/storybookjs/storybook-astro/pull/1
- Opened docs: Update links to actions docs page nextjs-server-example-app#2
Did not open for solidjs as it's already archived, but I know someone else has taken over a copy of the repo. We might have expired links in projects like the solid, nuxt, twig integrations. If possible, it could be useful to create a redirection for those as they'll be harder to track.
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 appreciate it, @Sidnioulz! I'll check those and do another search (including some of the Chromatic ones) to see if we missed anything. Regarding SolidJS, as it's been moved into the SolidJS community org, I'll reach out to the maintainer for an update ( or push it myself).
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.
@Sidnioulz appreciate you addressing the feedback so promptly. Appreciate it 🙏 !
Just a couple of minor items:
- Can you take a pass at the documentation and fix the spacing, as there are still some inconsistencies
- Regarding this comment, are we going with the currently documented approach? Just wanted a quick sanity check to ensure we didn't miss anything here.
Approving to unblock you as these are not blocking.
Hm, there might have been a quirk with Git. I had applied your suggestions and created additional commits to handle the feedback on spacing and snippets. Let me double check history as Norbert has pushed on the branch too. EDIT: I hadn't seen that my push got blocked due to the rebase commit. Repushing! |
|
Appreciate it 🙏 |
Co-authored-by: jonniebigodes <[email protected]>
Co-authored-by: jonniebigodes <[email protected]>
Co-authored-by: jonniebigodes <[email protected]>
1ab61d4 to
951debc
Compare
Co-authored-by: jonniebigodes <[email protected]>
code/lib/cli-storybook/src/automigrate/fixes/migrate-addon-console.ts
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,374 @@ | |||
| import { readFileSync, writeFileSync } from 'node:fs'; | |||
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.
As a reviewer, I believe that this tests all the functionality, but I do have an not-so-easy-time grokking the tests.
I suspect the 99% complexity that we want to test is in transformPreviewFile, isn't it?
Which is a string=>string transformer function, right?
Instead of going into fine detail, can we write a few tests where we assert
If "A" goes in "B" comes out; using toMatchSnapshot to assert 'correctness'.
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.
By splitting up the tests between units:
- When testing the automigration code, we check if
transformPreviewFileis called with the correct data. - When testing
transformPreviewFilewe check if the transform happens correctly
.. it should be easier to understand the intent, and quicker to address bugs.
WDYT @Sidnioulz ?
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've matched the structure of existing automigrations (IIRC from the a11y addon) when setting this up. I should've made a function that handles ASTs in retrospect.
- Good point, I'll build a collection of inputs and expected outputs to test
transformPreviewFileto make it easier to read.
I could add a test that checks that transformPreviewFile gets called with the preview file's content, but I'd need to shuffle files around to be able to mock the function, and then I'd have multiple files for this migration (which I see is not the pattern applied here, they tend to be self-contained files).
I think this test would have less value, considering we're more likely to make changes inside the transform function rather changes on how it's called in the lifecycle of this migration.
Co-authored-by: Norbert de Langen <[email protected]>
See issue #31847.
See alternative option #31847.
What I did
I added an automigration that implements @kasperpeulen's idea of replacing the addon with
spyOnmethods. Thanks @yannbf for showing me around!The automigration can help people migrate away from addon-console while upgrading to SB 9, and removes the need for specific development within the actions addon or core package.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Testing the automigration
yarn build --watch storybook sb-cliTesting the actions filter mechanism
PATCH
Documentation
Caution
TODOs left:
Checklist for Maintainers
When this PR is ready for testing, make sure to add
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-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
Greptile Summary
This PR introduces an automigration tool to help users transition away from
@storybook/addon-consolein Storybook 9.0. The key changes include:spyOnmethods from@storybook/testConfigFileclass to handle import/require statement removalThe implementation automatically detects addon-console usage, modifies preview files to add console spies, and removes the addon-console dependency. This change aligns with Storybook's goal of streamlining the addon ecosystem by moving console functionality into core features.
Confidence score: 4/5
6 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile