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

stories dir is not being type checked during Storybook build #668

Closed
dgreene1 opened this issue Apr 7, 2020 · 8 comments · Fixed by #876
Closed

stories dir is not being type checked during Storybook build #668

dgreene1 opened this issue Apr 7, 2020 · 8 comments · Fixed by #876
Assignees
Labels
kind: feature New feature or request scope: templates Related to an init template, not necessarily to core (but could influence core) solution: workaround available There is a workaround available for this issue

Comments

@dgreene1
Copy link

dgreene1 commented Apr 7, 2020

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)

Suggested solution(s)

Modify the .storybook/main.js in the templates and or the tsconfig.json so that ts-loader knows that it should be type checking the files.

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
Copy link
Collaborator

agilgur5 commented Apr 7, 2020

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
Copy link
Collaborator

agilgur5 commented Apr 8, 2020

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 agilgur5 added the solution: intended behavior This is not a bug and is expected behavior label Apr 8, 2020
@agilgur5
Copy link
Collaborator

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

@kylemh
Copy link
Contributor

kylemh commented Apr 26, 2020

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: #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
Copy link
Collaborator

agilgur5 commented Sep 20, 2020

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
Copy link
Author

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

@kylemh
Copy link
Contributor

kylemh commented Sep 21, 2020

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
Copy link
Contributor

kylemh commented Sep 21, 2020

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

@agilgur5 agilgur5 added solution: workaround available There is a workaround available for this issue scope: templates Related to an init template, not necessarily to core (but could influence core) kind: feature New feature or request and removed solution: intended behavior This is not a bug and is expected behavior labels Sep 21, 2020
@agilgur5 agilgur5 changed the title stories dir is not being type checked stories dir is not being type checked during Storybook build Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request scope: templates Related to an init template, not necessarily to core (but could influence core) solution: workaround available There is a workaround available for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants