Conversation
BREAKING CHANGE: Now requires Storybook 9.
package.json
Outdated
| { | ||
| "name": "@storybook/addon-styling-webpack", | ||
| "version": "1.0.1", | ||
| "version": "2.0.0", |
There was a problem hiding this comment.
Please note: I've bumped the version as I saw Shaun was doing it himself and I suspect it's not auto-managed. If version is managed via release commits (using Shaun's identity 😬), this will need to be reverted.
There was a problem hiding this comment.
Mental note: must definitely be reverted. Will do it later so as not to mess up with your branch history while you're working on this, Norbert.
There was a problem hiding this comment.
Pull Request Overview
This PR ports the addon to Storybook 9 by bumping Node/TS build targets, updating Storybook imports, refining the postinstall helper, and upgrading package metadata.
- Bump Node target and runtime version to 20.
- Update webpackFinal to use Storybook 9 internal APIs and refine style-rule filtering.
- Refactor postinstall script, tighten package.json versions, and raise major version to 2.0.0.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tsup.config.js | Updated build target from node16 to node20 |
| src/webpackFinal.ts | Switched import to new Storybook internal logger path and enhanced rule filtering |
| src/preset.ts | Removed unnecessary as any cast for webpackFinal |
| src/postinstall.ts | Introduced spawnAsync helper, improved PM command typing, and switched to asynchronous spawn |
| package.json | Bumped to v2.0.0, added Storybook 9 devDependency, updated exports |
| .nvmrc | Updated Node.js version from 18 to 20 |
Comments suppressed due to low confidence (3)
src/postinstall.ts:9
- [nitpick] The variable name 'process' shadows the global Node 'process' object; consider renaming it to 'childProcess' or similar to avoid confusion.
const process = spawn(cmd, args, options);
package.json:61
- This addon uses Storybook 9 APIs at runtime but does not declare Storybook as a peerDependency; consider adding the appropriate '@storybook/*' package to peerDependencies to ensure compatibility.
"peerDependencies": {
src/webpackFinal.ts:1
- Importing from 'storybook/internal/node-logger' relies on internal APIs that could change; use the public '@storybook/node-logger' package instead to ensure compatibility.
import { logger, colors } from "storybook/internal/node-logger";
| "types": "./dist/index.d.ts", | ||
| "require": "./dist/index.js", | ||
| "import": "./dist/index.mjs", | ||
| "types": "./dist/index.d.ts" | ||
| "import": "./dist/index.mjs" | ||
| }, | ||
| "./preset": { | ||
| "types": "./dist/preset.d.ts", | ||
| "require": "./dist/preset.js", | ||
| "import": "./dist/preset.mjs", | ||
| "types": "./dist/preset.d.ts" | ||
| "import": "./dist/preset.mjs" |
There was a problem hiding this comment.
Fixed import order warning in more recent bundlers
| "auto": "^11.3.0", | ||
| "prettier": "^3.5.3", | ||
| "rimraf": "^6.0.1", | ||
| "storybook": "^9.0.4", |
There was a problem hiding this comment.
Instead of node-logger which has been internalised.
| const spawnAsync = ( | ||
| cmd: Parameters<typeof spawn>[0], | ||
| args: Parameters<typeof spawn>[1], | ||
| options: Parameters<typeof spawn>[2], | ||
| ): Promise<void> => { | ||
| return new Promise((resolve, reject) => { | ||
| const process = spawn(cmd, args, options); | ||
|
|
||
| process.on("close", (code) => { | ||
| if (code === 0) { | ||
| resolve(); | ||
| } else { | ||
| reject(new Error(`Process exited with code ${code}`)); | ||
| } | ||
| }); | ||
|
|
||
| process.on("error", (err) => { | ||
| reject(err); | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Fixing warning about awaiting sync code. This is the proper way to wait for a child spawn exec.
This being said, some (very few) files in the main repo use cross-spawn. Should its use be generalised?
| import { webpackFinal as webpack } from "./webpackFinal"; | ||
|
|
||
| export const webpackFinal = webpack as any; | ||
| export const webpackFinal = webpack; |
There was a problem hiding this comment.
Removed unnecessary cast. If the goal is to strip type information for end users, how about using unknown instead to show this isn't a type inference issue?
| // Remove any existing rules for styles | ||
| config.module.rules = config.module.rules.filter( | ||
| (rule) => typeof rule === "object" && !isRuleForStyles(rule) | ||
| (rule) => typeof rule === "object" && rule && !isRuleForStyles(rule), |
There was a problem hiding this comment.
Handling null case as isRuleForStyles doesn't.
…s in pnpm-lock.yaml
…entry in package.json
|
This seems ready to me @Sidnioulz 🎉 |
BREAKING CHANGE: Now requires Storybook 9.
Please note: I don't know test this code as it has no UT or local stories and I haven't used Webpack in a hot minute. What's the usual process like for this addon?
📦 Published PR as canary version:
2.0.0--canary.31.6935319.0✨ Test out this PR locally via:
npm install @storybook/addon-styling-webpack@2.0.0--canary.31.6935319.0 # or yarn add @storybook/addon-styling-webpack@2.0.0--canary.31.6935319.0