Skip to content
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

[Bug]: React+Vite+TS PnP breaks type in .storybook/main.ts > config.framework.name #23232

Closed
sni-J opened this issue Jun 27, 2023 · 4 comments · Fixed by #27088 or #27180
Closed

[Bug]: React+Vite+TS PnP breaks type in .storybook/main.ts > config.framework.name #23232

sni-J opened this issue Jun 27, 2023 · 4 comments · Fixed by #27088 or #27180

Comments

@sni-J
Copy link
Contributor

sni-J commented Jun 27, 2023

Describe the bug

Storybook in yarn berry PnP & Vite (React + TS) creates type error in .storybook/main.ts
(and other vite frameworks might have same issue)

Initializing storybook generates following code in .storybook/main.ts...

import type { StorybookConfig } from "@storybook/react-vite";

const config: StorybookConfig = {
  // ...
  framework: {
    name: path.dirname(require.resolve(path.join("@storybook/react-vite", "package.json"))),
    options: {},
  },
  //...

... which are generated by ...

framework: { name: frameworkInclude, options: options.framework || {} },
const {
packages: frameworkPackages,
type,
rendererId,
framework: frameworkInclude,
builder: builderInclude,
} = getFrameworkDetails(renderer, builder, pnp, framework);
const getFrameworkDetails = (
renderer: SupportedRenderers,
builder: Builder,
pnp: boolean,
framework?: SupportedFrameworks
): {
type: 'framework' | 'renderer';
packages: string[];
builder?: string;
framework?: string;
renderer?: string;
rendererId: SupportedRenderers;
} => {
const frameworkPackage = getFrameworkPackage(framework, renderer, builder);
const frameworkPackagePath = pnp ? wrapForPnp(frameworkPackage) : frameworkPackage;
const wrapForPnp = (packageName: string) =>
`%%path.dirname(require.resolve(path.join('${packageName}', 'package.json')))%%`;

... and this conflicts with type of framework.name in StorybookConfig

type FrameworkName = '@storybook/react-vite';

Code above added in #20642, looks like PnP case was not considered.

Originally, type of framework.name, which was referring StorybookConfig from @storybook/types, was string and PR above narrowed it down to each framework's name

/**
* Framework, e.g. '@storybook/react-vite', required in v7
*/
framework?: Preset;
export type Preset =
| string
| {
name: string;
options?: any;
};

AFAIK about typescript, it seems like type FrameworkName = string is the only way to solve this issue, but not so sure.
And it might lose advantages from strict typing.
(IMHO I love strict typing but in most case those values are created by generator so it doesn't look like huge advantage though)

Are there any other better solutions?


For somebody confronted same issue,
to bypass before this issue resolved, use @ts-expect-error like below.

// @ts-expect-error PnP resolution conflicts string literal type
name: path.dirname(require.resolve(path.join("@storybook/react-vite", "package.json"))),

To Reproduce

  1. Yarn berry with nodeLinker: pnp
  2. yarn create vite --template react-ts
  3. yarn dlx storybook@latest init

System

--

Additional context

--

@shilman
Copy link
Member

shilman commented Sep 20, 2023

@kasperpeulen is there an easy fix for this in TS metaprogramming land?

@kasperpeulen
Copy link
Contributor

@shilman Should be doable, I actually thought @valentinpalkovic and @ndelangen already worked on this issue?

@ndelangen
Copy link
Member

@kasperpeulen I thought you had some TypeScript trick to make something both a string but also keep the ts suggestions.

Do we think that's worth doing here? It it helpful?

@kasperpeulen
Copy link
Contributor

I think it is helpful: 'literal' | 'other' | (string & {}) does the trick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
5 participants