Core: Fix builder stats typings to be optional

[AriPerkkio]

Issue: storybookjs/builder-vite#397

What I did

Fixed typings of Builder (@storybook/core-common package). Currently typings and implementation do not match. The implementation is using @ts-ignore 😭

storybook/lib/core-server/src/build-dev.ts

Lines 109 to 111 in 61d6451

	// I'm a little reticent to import webpack types in this file :shrug:
	// @ts-ignore
	const previewWarnings = (previewStats && previewStats.toJson().warnings) || [];

Usage of Builder<SomeValidConfigType, {}> causes TypeErrors mentioned in the linked issue. If typings were correct these kind of issues would be prevented.

How to test

• Is this testable with Jest or Chromatic screenshots?
• Does this need a new example in the kitchen sink apps?
• Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b49b835. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 1 target

• nx run-many --target=prepare --all --parallel=8 --max-parallel=15 -- --reset --optimized

---

Sent with 💌 from NxCloud.

[ndelangen]

It also seems that this causes 2 issues:

> nx run @storybook/manager-webpack5:prepare --optimized 
ERR! FAILED (ts) : src/index.ts:18:46 - error TS2344: Type 'import("/tmp/storybook/lib/manager-webpack5/node_modules/webpack/types").Stats' does not satisfy the constraint 'import("/tmp/storybook/node_modules/@types/webpack/index").Stats'.
  Type 'Stats' is missing the following properties from type 'Stats': formatFilePath, normalizeFieldKey, sortOrderRegular

18 type WebpackBuilder = Builder<Configuration, Stats>;
                                                ~~~~~
Found 1 error

> nx run @storybook/builder-webpack5:prepare --optimized 
ERR! FAILED (ts) : src/index.ts:11:46 - error TS2344: Type 'import("/tmp/storybook/lib/builder-webpack5/node_modules/webpack/types").Stats' does not satisfy the constraint 'import("/tmp/storybook/node_modules/@types/webpack/index").Stats'.
  Type 'Stats' is missing the following properties from type 'Stats': formatFilePath, normalizeFieldKey, sortOrderRegular

11 type WebpackBuilder = Builder<Configuration, Stats>;
                                                ~~~~~
Found 1 error.

[AriPerkkio]

It also seems that this causes 2 issues:
[...] Type 'Stats' is missing the following properties from type 'Stats': formatFilePath, normalizeFieldKey, sortOrderRegular

Good catch. I was unable to run storybook locally when creating the PR but got it running now - I'm also seeing this error now.

Looks like it's caused by Webpack v4 vs v5 Stats typings. Is there a way to have two versions of webpack's typings in @storybook/core-common? Then simply use type BuilderStats = Webpack4Stats | Webpack5Stats;. Currently the types are coming from here:

storybook/lib/core-common/package.json

Line 92 in 008b542

"webpack": "4"

storybook/lib/core-common/src/types.ts

Line 4 in 008b542

import type { Configuration, Stats } from 'webpack';

Another option is to create a custom interface where we explicitly list the types we expect from a builder stats. This would mean that whole codebase should be scanned for the usage of stats - as there are @ts-ignore's used, these won't come up automatically, I think.
Something like:

/** Webpack 4/5 compatible `Stats` interface */
interface Stats {
    /** Returns compilation information as a JSON object. */
    toJson(options?: SomeType, forToString?: boolean): SomeOutputType;
}

[ndelangen]

@AriPerkkio I've been working on future/base and a series of PRs that will all be part of 7.0..
The whole webpack 4 & 5 support and having it in 1 monorepo is a pain, believe me I know... this (part of)is why in 7.0 we'll drop webpack 4 support.

Perhaps you'll be keen to take a look at my work here:
#18504

Do you think it'd be best to focus your efforts on 7.0 as well?

FYI: The default branch next is in a in between state between 6.5 and 7.0, where most work will become part of 7.0 anyway.
But it helps us keep merge conflicts to a minimum so we can cherry-pick important fixes to main and thus release in 6.5.x

[AriPerkkio]

Do you think it'd be best to focus your efforts on 7.0 as well?

Yes, I think we should only target the 7.0 here and Webpack 5 only. It makes thing a lot easier.

However it seems that in #18504 the @storybook/core-common does not have Webpack typings anymore. There is a new package, @storybook/core-webpack, which handles those. @storybook/core-common is still the one that defines Builder
s stats:

storybook/lib/core-common/src/types.ts

Line 193 in 9b92429

export interface Builder<Config, Stats> {

The idea of separating Webpack typings from @storybook/core-common is great but those stats are still a Webpack thing. I don't think we want to make @storybook/core-common dependent of @storybook/webpack-common, right? Where should the typings of stats be defined?

[ndelangen]

They should be super generic in core-common then be extended and made webpack specific in core-webpack, and then possibly be extended again in builder-webpack5.

[ndelangen]

@AriPerkkio let's rebase this unto future/base and proceed from there? Would you need my help with that?

[AriPerkkio]

Rebased future/base and changed that one to be PR target branch. It already seems to have generic typings for Stats - this is perfect:

storybook/lib/core-common/src/types.ts

Lines 102 to 104 in 379d8b8

	export interface Stats {
	toJson: () => any;
	}

Now, with changes of this PR, builders which do not provide such Stats, can utilize the Builder interface without providing dummy Stats, e.g. Builder<ViteConfig>. I also removed the @ts-ignore's from the code now that everything is properly typed. There should be no more null pointers caused by falsely typed builders!

[ndelangen]

Awesome, I'll take a look on Monday!

[ndelangen]

Sorry for the delay, I merged in the base branch, to see if the CI approves... If it does, I'm going to merge it! 👏👏👏