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]: @storybook/csf-tools doesn't parse .storybook/main.ts config properly if using satisfies StorybookConfig #20935

Closed
stevezhu opened this issue Feb 4, 2023 · 5 comments · Fixed by #20936

Comments

@stevezhu
Copy link

stevezhu commented Feb 4, 2023

Describe the bug

Issue

If using typescript storybook config and satisfies from typescript 4.9 to typecheck

export default {
  // config here
} satisfies StorybookConfig;

instead of something like

const config: StorybookConfig = {
  // config here
};
export default config;

the ast isn't handled properly.

As you can see below in the asts under "Additional context", when using the satisfies operator, the ObjectExpression is wrapped in any extra TSSatisfiesExpression node which causes the code to break.

A new unit test is probably needed in https://github.com/storybookjs/storybook/blob/next/code/lib/csf-tools/src/ConfigFile.test.ts that tests the satisfies operator.

My storybook config for example:

.storybook/main.ts

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

export default {
  stories: [],
  addons: [
    '@storybook/addon-essentials',
  ],
  framework: {
    name: '@storybook/react-vite',
    options: {},
  },
} satisfies StorybookConfig; // issue here

Root Cause

This began in version v7.0.0-beta.39.

The following call seems to spam the warning log from the following line starting in v7.0.0-beta.39.
https://github.com/storybookjs/storybook/blob/v7.0.0-beta.39/code/lib/csf-tools/src/ConfigFile.ts#L155

      ExportDefaultDeclaration: {
        enter({ node, parent }) {
          self.hasDefaultExport = true;
          const decl =
            t.isIdentifier(node.declaration) && t.isProgram(parent)
              ? _findVarInitialization(node.declaration.name, parent)
              : node.declaration;

          if (t.isObjectExpression(decl)) {
            self._exportsObject = decl;
            decl.properties.forEach((p: t.ObjectProperty) => {
              const exportName = propKey(p);
              if (exportName) {
                let exportVal = p.value;
                if (t.isIdentifier(exportVal)) {
                  exportVal = _findVarInitialization(exportVal.name, parent as t.Program);
                }
                self._exports[exportName] = exportVal as t.Expression;
              }
            });
          } else {
            logger.warn(`Unexpected ${JSON.stringify(node)}`); // spam logging here
          }
        },
      },

This is probably because of a newly added call to readConfig here that ends up hitting the previous piece of code, which didn't exist in v7.0.0-beta.38.
https://github.com/storybookjs/storybook/blob/v7.0.0-beta.39/code/lib/core-common/src/utils/validate-configuration-files.ts#L28

    const main = await readConfig(mainConfigPath);

Temporary workaround (if using pnpm)

Add to package.json

{
  "pnpm": {
    "overrides": {
      "@storybook/core-common": "7.0.0-beta.38"
    }
  }
}

To Reproduce

Can probably reproduce just by adding a new test to https://github.com/storybookjs/storybook/blob/next/code/lib/csf-tools/src/ConfigFile.test.ts that uses the satisfies operator`.

System

Environment Info:

  System:
    OS: macOS 13.1
    CPU: (8) x64 Intel(R) Core(TM) i5-1038NG7 CPU @ 2.00GHz
  Binaries:
    Node: 19.5.0 - /usr/local/bin/node
    npm: 9.3.1 - /usr/local/bin/npm
  Browsers:
    Chrome: 109.0.5414.119
    Firefox: 90.0.2
    Safari: 16.2

Additional context

Unsupported AST (with satisfies)

Node {
  type: 'TSSatisfiesExpression',
  start: 78,
  end: 467,
  loc: null,
  expression: Node {
    type: 'ObjectExpression',
    start: 78,
    end: 441,
    loc: SourceLocation {
      start: [Position],
      end: [Position],
      filename: undefined,
      identifierName: undefined,
      lines: [Lines],
      tokens: [Array],
      indent: 0
    },
    properties: [ [Node], [Node], [Node], [Node] ],
    extra: { trailingComma: 438 }
  },
  typeAnnotation: Node {
    type: 'TSTypeReference',
    start: 452,
    end: 467,
    loc: SourceLocation {
      start: [Position],
      end: [Position],
      filename: undefined,
      identifierName: undefined,
      lines: [Lines],
      tokens: [Array],
      indent: 0
    },
    typeName: Node {
      type: 'Identifier',
      start: 452,
      end: 467,
      loc: [SourceLocation],
      name: 'StorybookConfig'
    }
  }
}

Supported AST (without satisfies)

Node {
  type: 'ObjectExpression',
  start: 78,
  end: 441,
  loc: null,
  properties: [
    Node {
      type: 'ObjectProperty',
      start: 82,
      end: 212,
      loc: [SourceLocation],
      method: false,
      key: [Node],
      computed: false,
      shorthand: false,
      value: [Node]
    },
    Node {
      type: 'ObjectProperty',
      start: 216,
      end: 330,
      loc: [SourceLocation],
      method: false,
      key: [Node],
      computed: false,
      shorthand: false,
      value: [Node]
    },
    Node {
      type: 'ObjectProperty',
      start: 334,
      end: 402,
      loc: [SourceLocation],
      method: false,
      key: [Node],
      computed: false,
      shorthand: false,
      value: [Node]
    },
    Node {
      type: 'ObjectProperty',
      start: 406,
      end: 438,
      loc: [SourceLocation],
      method: false,
      key: [Node],
      computed: false,
      shorthand: false,
      value: [Node]
    }
  ],
  extra: { trailingComma: 438 }
}
@zhyd1997
Copy link
Contributor

zhyd1997 commented Feb 5, 2023

You need to migrate the main.ts configuration if you're using v7.0 and the 2nd code snippet in your issue description is correct and the only way to typecheck the config file.

https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#esm-format-in-mainjs

storybook/MIGRATION.md

Lines 325 to 335 in 293d52c

For Typescript users, we introduced types for that default export, so you can import it in your main.ts file. The `StorybookConfig` type will come from the Storybook package for the framework you are using, which relates to the package in the "framework" field you have in your main.ts file. For example, if you are using React Vite, you will import it from `@storybook/react-vite`:
```ts
import { StorybookConfig } from '@storybook/react-vite';
const config: StorybookConfig = {
stories: ['../stories/**/*.stories.mdx', '../stories/**/*.stories.@(js|jsx|ts|tsx)'],
framework: { name: '@storybook/react-vite' },
};
export default config;
```

Screenshot 2023-02-05 at 08 37 58

@stevezhu
Copy link
Author

stevezhu commented Feb 5, 2023

@zhyd1997 Yeah that is the only way currently, but I think it makes sense that satisfies is also supported. I wouldn't necessarily call the current suggestion the correct way. For these config type files, using satisfies provides a much more accurate resultant type than typing the entire thing as StorybookConfig.

@zhyd1997
Copy link
Contributor

zhyd1997 commented Feb 5, 2023

Hi @stevezhu

const config = {
  // ...
}

export default config satisfies StorybookConfig

this way also works.

@stevezhu
Copy link
Author

stevezhu commented Feb 5, 2023

@zhyd1997 Thanks for the suggestions I'm aware of how to fix it, this issue is more of a feature request for satisfies support for configs. I would prefer to be able to export it directly without the intermediate declaration.

Also for context on this thread, there's satisfies support for csf meta #20033 so it would be consistent to also support it for configs.

@shilman
Copy link
Member

shilman commented Feb 7, 2023

Hurrah!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.44 containing PR #20936 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants