stories dir is not being type checked during Storybook build

dgreene1 commented 4 years ago

Current Behavior

If I have bad typescript code in /stories/button.stories.tsx (like the following code) I will not get a typescript build error when I'm either watching storybook or building storybook:

const falseAlways = false;
falseAlways = true; // <-- shouldn't able to reassign a const & also a false literal can't be set to true.

Expected behavior

I should get a build error when running build-storybook that says: Cannot assign to 'falseAlways' because it is a constant.ts(2588)

Additional context

This might be a problem due to the noEmit flag in tsconfig.json; however, I've tried turning file emitting back on and just simple directing it (via outDir) to a different directory.

I've used the default main.js but I've also tried adding to it to see if I can get better feedback:

const WebpackNotifierPlugin = require('webpack-notifier');
const WarningsToErrorsPlugin = require('warnings-to-errors-webpack-plugin');

module.exports = {
  stories: ['../stories/**/*.stories.(ts|tsx)'],
  addons: [
    '@storybook/addon-actions',
    '@storybook/addon-links',
    '@storybook/addon-docs',
  ],
  webpackFinal: async config => {

    console.log(`#####################################
      Please forgive the wait. The startup time is slow but the watcher time should be quite quick.
    #####################################`);

    const compilerOptions = require("../tsconfig.build.json").compilerOptions;

    if(!compilerOptions){
      throw new Error("Could not find tsconfig.json");
    }

    // include the stories so we can get type checking
    compilerOptions.rootDirs = compilerOptions.rootDirs || [];
    if(compilerOptions.rootDir){
      console.warn("Modifying rootDir to be a rootDirs array so we can get type checking for the stories");
      compilerOptions.rootDirs.push(compilerOptions.rootDir);
      compilerOptions.rootDir = "..";
    }
    compilerOptions.rootDirs.push("./stories");
    compilerOptions.outDir = "./storybook-static";
    compilerOptions.isolatedModules = false;
    // When building for storybook we need to override this setting or we'd get an error
    delete compilerOptions.moduleResolution;

    config.module.rules.push({
      test: /\.(ts|tsx)$/,
      use: [
        {
          loader: require.resolve('ts-loader'),
          options: {
            compilerOptions
          }
        },
        {
          loader: require.resolve('react-docgen-typescript-loader'),
          options: {
            compilerOptions
          }
        },
      ],
    });
    config.module.rules.push({
      test: /\.s[ac]ss$/i,
      use: [
        // Creates `style` nodes from JS strings
        'style-loader',
        // Translates CSS into CommonJS
        'css-loader',
        // Compiles Sass to CSS
        'sass-loader',
      ],
    });

    config.resolve.extensions.push('.ts', '.tsx');

    config.plugins = config.plugins || [];
    config.plugins.push(
      new WebpackNotifierPlugin({ title: 'Custom Storybook Webpack' })
    );
    config.plugins.push(new WarningsToErrorsPlugin());

    return config;
  },
};

Your environment

Software	Version(s)
TSDX	0.13.1
TypeScript	3.8.3
Browser	n.a. but Chrome
npm/Yarn	npm 6.13.4
Node	12.15.1
Operating System	Windows 10

agilgur5 commented 4 years ago

Ah, this was one of the things I was worried about in #646 / the fix to #638 , that files outside of src/ wouldn't be type-checked. Stories weren't type-checked previously either, so this isn't a result of that and is actually a separate bug; I was concerned about tests there, which were previously type-checked.

Per the PR, I wasn't able to repro that in editor, but sounds like build-storybook repros it. I'll need to verify this on my own with the template since you have a custom config here.

The fix is to have a separate tsconfig.build.json that's used for tsdx build and use root tsconfig.json just for type-checking. That's a fairly big change though, so I didn't want to do that without significant reason to do so.

More specifically, the missing piece here is to change include in the tsconfig to also have the stories dir. include decides what gets compiled. I'd think Storybook would override include with the stories option though, so that might be an upstream bug to fix (TS support is still WIP in Storybook).

We don't want that include for tsdx build though (as then you'd have type declarations generated in your dist for stories), so that's where the tsconfig.json vs. tsconfig.build.json separation becomes a thing.

agilgur5 commented 4 years ago

Ok, well I tried and I couldn't reproduce this with the template config because the template config has transpileOnly turned on (I'm not entirely sure why the intent behind that was in #435 though, as it was a change from the old behavior).

I then tried removing that as well and temporarily adding stories to include and temporarily changing rootDir to ./ and it still didn't report any type errors... Not sure why that is.

Pinging our "resident Storybook expert" (😉 ) @kylemh for input on both of the above

agilgur5 commented 4 years ago

Pinging @kylemh again as haven't gotten responses in multiple threads now... 😕

kylemh commented 4 years ago

Sorry about being so silent. Been crazy busy with other objectives. Using TSDX with Storybook is gonna be a priority for me real soon at work, and that'll be when I fix the storybook template, unless somebody else takes a swing!

The storybook typescript preset may automatically resolve the issues, but there are a lot of snafus if used with Storybook v5.

See: https://github.com/jaredpalmer/tsdx/issues/440 and https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#react-prop-tables-with-typescript for more

Hoping to wait until Storybook v6 is released...

@agilgur5 @sw-yx any chance of adding me as a collaborator and assigning me the Storybook issues?

agilgur5 commented 4 years ago

So has this been resolved in Storybook v6? I think it has since transpileOnly is no longer in the config, but not sure as I have not tried or know what the upstream default config looks like

dgreene1 commented 4 years ago

@agilgur5 my coworker @yhy-vertex will be investigating this in the next week or two. We’ll update you on our findings. :)

kylemh commented 4 years ago

I added an invalid prop and value to a story here (not committed) and was able to build storybook and use it's dev server without type errors, so my guess is that its hasn't been resolved.

kylemh commented 4 years ago

Ah! Looks like it does work! You just need to adjust the config in .storybook/main.js, such that check is true.

https://storybook.js.org/docs/react/configure/typescript#mainjs-configuration

jaredpalmer / tsdx