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.

Sign up for GitHub

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

Jump to bottom

[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
Labels
bug good first issue help wanted typescript

Comments

@sni-J
Copy link
Contributor

sni-J commented Jun 27, 2023 •
edited

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

storybook/code/lib/cli/src/generators/baseGenerator.ts

Line 365 in 243ca40

framework: { name: frameworkInclude, options: options.framework || {} },

storybook/code/lib/cli/src/generators/baseGenerator.ts

Lines 223 to 229 in 243ca40

const {
packages: frameworkPackages,
type,
rendererId,
framework: frameworkInclude,
builder: builderInclude,
} = getFrameworkDetails(renderer, builder, pnp, framework);

storybook/code/lib/cli/src/generators/baseGenerator.ts

Lines 105 to 120 in 243ca40

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;

storybook/code/lib/cli/src/generators/baseGenerator.ts

Lines 102 to 103 in 243ca40

const wrapForPnp = (packageName: string) =>
`%%path.dirname(require.resolve(path.join('${packageName}', 'package.json')))%%`;

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

storybook/code/frameworks/react-vite/src/types.ts

Line 8 in 243ca40

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

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

Lines 325 to 328 in 4f7342b

/**
* Framework, e.g. '@storybook/react-vite', required in v7
*/
framework?: Preset;

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

Lines 226 to 231 in 4f7342b

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

@valentinpalkovic valentinpalkovic mentioned this issue May 10, 2024
Merged
8 tasks
@sni-J sni-J mentioned this issue May 17, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue help wanted typescript
Projects
Core Team Projects
Status: Done
Milestone
No milestone
5 participants
@shilman @kasperpeulen @ndelangen @valentinpalkovic @sni-J