Skip to content

Conversation

@Sidnioulz
Copy link
Member

@Sidnioulz Sidnioulz commented Jul 19, 2025

See issue #31847.
See alternative option #31847.

What I did

I added an automigration that implements @kasperpeulen's idea of replacing the addon with spyOn methods. 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
  1. Checkout code and yarn build --watch storybook sb-cli
  2. Clone the testbed repo
  3. Check out the commands on the testbed repo to go through all 4 branches and test the automigration script
Testing the actions filter mechanism
  1. Check out my branch
  2. Copy and apply the patch below
  3. Go to http://localhost:6006/?path=/story/core-spies--show-spy-on-in-actions
  4. Check that console.log shows actions 1 and 2 (and action 3 when clicking the button), but that console.warn only shows action 1 (and 3 when clicking); action 2 is filtered out
PATCH
From 76949c2205b84066cce8a8cdd3182536af0761ce Mon Sep 17 00:00:00 2001
From: Steve Dodier-Lazaro <[email protected]>
Date: Tue, 29 Jul 2025 21:48:48 +0200
Subject: [PATCH] Debug patch - test actions with a filtered spy

---
 code/.storybook/preview.tsx                 | 20 +++++++++++-
 code/core/template/stories/spies.stories.ts | 34 ++++++++++++++++++++-
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/code/.storybook/preview.tsx b/code/.storybook/preview.tsx
index 5ad330df65a..1e631d84954 100644
--- a/code/.storybook/preview.tsx
+++ b/code/.storybook/preview.tsx
@@ -17,9 +17,10 @@ import addonThemes from '@storybook/addon-themes';
 import addonTest from '@storybook/addon-vitest';
 
 import addonPseudoStates from 'storybook-addon-pseudo-states';
+import { action } from 'storybook/actions';
 import { DocsContext as DocsContextProps, useArgs } from 'storybook/preview-api';
 import type { PreviewWeb } from 'storybook/preview-api';
-import { sb } from 'storybook/test';
+import { sb, spyOn } from 'storybook/test';
 import {
   Global,
   ThemeProvider,
@@ -394,6 +395,22 @@ const parameters = {
   },
 };
 
+const beforeEach = async () => {
+  const originalConsoleWarn = console.warn;
+
+  spyOn(console, 'warn')
+    .mockName('')
+    .mockImplementation((...args) => {
+      // Check if the warn message matches a certain pattern
+      if (args[0].startsWith('hello')) {
+        action('console.warn')(args);
+      }
+
+      // Call the original console.warn function
+      originalConsoleWarn(...args);
+    });
+};
+
 export default definePreview({
   addons: [
     addonDocs(),
@@ -403,6 +420,7 @@ export default definePreview({
     addonPseudoStates(),
     templatePreview,
   ],
+  beforeEach,
   decorators,
   loaders,
   tags: ['test', 'vitest'],
diff --git a/code/core/template/stories/spies.stories.ts b/code/core/template/stories/spies.stories.ts
index e74f89c95c7..1069f8fe91a 100644
--- a/code/core/template/stories/spies.stories.ts
+++ b/code/core/template/stories/spies.stories.ts
@@ -1,28 +1,60 @@
 import { global as globalThis } from '@storybook/global';
 
-import { spyOn } from 'storybook/test';
+import { fn, spyOn } from 'storybook/test';
 
 const meta = {
   component: globalThis.__TEMPLATE_COMPONENTS__.Button,
   beforeEach() {
     spyOn(console, 'log').mockName('console.log');
     console.log('first');
+    console.warn('hello world (1/3)');
+  },
+  argTypes: {
+    onClick: {
+      action: 'clicked',
+    },
   },
 };
 
 export default meta;
 
+// This one should display clicked: {__isClassInstance__: true, __className__: "SyntheticBaseEvent", _reactName: "onClick", _targetInst: null, type: "click"…}
+export const DefaultStoryArgSpies = {
+  parameters: {
+    chromatic: { disable: true },
+  },
+  args: {
+    label: 'Button',
+  },
+};
+
+// This one should display "onClick" with the parameters; it does but the event appears twice for some reason.
+export const StoryArgSpiesWithUserSuppliedFunction = {
+  parameters: {
+    chromatic: { disable: true },
+  },
+  args: {
+    label: 'Button',
+    onClick: fn().mockImplementation(() => {
+      const a = 'b';
+    }),
+    // onClick: fn(),
+  },
+};
+
 export const ShowSpyOnInActions = {
   parameters: {
     chromatic: { disable: true },
   },
   beforeEach() {
     console.log('second');
+    console.warn('hi world (2/3)');
   },
   args: {
     label: 'Button',
     onClick: () => {
       console.log('third');
+      console.warn('hello and good bye (3/3)');
     },
   },
 };
-- 
2.50.1

Documentation

Caution

TODOs left:

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts

  • Make 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-console in Storybook 9.0. The key changes include:

  1. A new migration script that replaces addon-console usage with spyOn methods from @storybook/test
  2. Enhanced functionality in ConfigFile class to handle import/require statement removal
  3. Integration with the CLI's automigration system

The 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

  1. This PR is safe to merge with thorough testing and clear rollback path
  2. High confidence due to comprehensive test coverage (unit + integration) and clear migration strategy, minor concerns about the FIXME comment in bin/index.ts
  3. Files needing attention:
    • code/lib/cli-storybook/src/bin/index.ts (FIXME comment needs resolution)
    • code/lib/cli-storybook/src/automigrate/fixes/migrate-addon-console.ts (method list needs review per FIXME)

6 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile

@Sidnioulz Sidnioulz changed the title feat(cli): Add addon-console automigration CLI: Add addon-console automigration Jul 19, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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

@nx-cloud
Copy link

nx-cloud bot commented Jul 19, 2025

View your CI Pipeline Execution ↗ for commit 8077930

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 45s View ↗

☁️ Nx Cloud last updated this comment at 2025-09-15 13:45:11 UTC

@storybook-app-bot
Copy link

storybook-app-bot bot commented Jul 19, 2025

Package Benchmarks

Commit: 8077930, ran on 15 September 2025 at 13:34:40 UTC

No significant changes detected, all good. 👏

@Sidnioulz Sidnioulz force-pushed the sidnioulz/addon-console-automigration branch 3 times, most recently from cf558cc to ca186e8 Compare July 21, 2025 10:37
@yannbf yannbf self-assigned this Jul 21, 2025
@yannbf
Copy link
Member

yannbf commented Jul 21, 2025

Idea: Write an FAQ entry in the module mocking/spies page about spying and showing actions in the panel. Simple case vs advanced

@Sidnioulz Sidnioulz force-pushed the sidnioulz/addon-console-automigration branch from 86363df to f6d27c1 Compare August 11, 2025 12:32
@Sidnioulz
Copy link
Member Author

For the record, both spies and fn on args work with the HTML framework. No need to add a warning in the doc.

@Sidnioulz
Copy link
Member Author

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.

Copy link
Contributor

@jonniebigodes jonniebigodes left a 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
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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?

Copy link
Member Author

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:

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.

Copy link
Contributor

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).

Copy link
Contributor

@jonniebigodes jonniebigodes left a 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.

@Sidnioulz
Copy link
Member Author

Sidnioulz commented Aug 20, 2025

@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!

@jonniebigodes
Copy link
Contributor

Appreciate it 🙏

@Sidnioulz Sidnioulz force-pushed the sidnioulz/addon-console-automigration branch from 1ab61d4 to 951debc Compare August 29, 2025 05:34
@Sidnioulz Sidnioulz assigned ndelangen and unassigned yannbf Sep 1, 2025
@Sidnioulz Sidnioulz requested a review from ndelangen September 1, 2025 13:51
@@ -0,0 +1,374 @@
import { readFileSync, writeFileSync } from 'node:fs';
Copy link
Member

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'.

Copy link
Member

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 transformPreviewFile is called with the correct data.
  • When testing transformPreviewFile we check if the transform happens correctly

.. it should be easier to understand the intent, and quicker to address bugs.

WDYT @Sidnioulz ?

Copy link
Member Author

@Sidnioulz Sidnioulz Sep 4, 2025

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 transformPreviewFile to 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.

@Sidnioulz Sidnioulz requested a review from ndelangen September 15, 2025 09:02
@ndelangen ndelangen added this to the 10 milestone Sep 15, 2025
@ndelangen ndelangen merged commit f8f2ef5 into next Sep 15, 2025
56 checks passed
@ndelangen ndelangen deleted the sidnioulz/addon-console-automigration branch September 15, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants