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

use @storybook/preset-typescript and support MDX on Storybook template #440

Closed
mariusespejo opened this issue Jan 22, 2020 · 16 comments · Fixed by #805
Closed

use @storybook/preset-typescript and support MDX on Storybook template #440

mariusespejo opened this issue Jan 22, 2020 · 16 comments · Fixed by #805
Labels
scope: templates Related to an init template, not necessarily to core (but could influence core) solution: outdated This is not up-to-date with the current version

Comments

@mariusespejo
Copy link

Current Behavior

  • The current additional webpack config used in the react with storybook template I believe is unnecessary
  • The template does not support Rich MDX docs out-of-the-box

Desired Behavior

  • Rather than providing custom webpack config, utilize Storybook's own typescript preset
  • Users should be able to easily add MDX-based stories/docs

Suggested Solution

First of all, I noticed that the config was recently cleaned up in #435 (thanks @kylemh !) and expanding on that work, I believe that by taking advantage of @storybook/preset-typescript, we should be able to replace the custom webpack config (can be seen in #435) like so:

const path = require('path');

module.exports = {
  stories: ['../stories/**/*.stories.(tsx|mdx)'], // notice mdx support added here
  presets: [
    {
      name: '@storybook/preset-typescript',
      options: {
        include: [
          path.resolve(__dirname, '../src'),
          path.resolve(__dirname, '../stories'),
        ],
      },
    },
  ],
  addons: [
    '@storybook/addon-actions',
    '@storybook/addon-links',
    '@storybook/addon-docs',
  ],
};

I believe that this will better support the tsdx goal of minimal/zero config. The above config still supports docgen extracting comments out of the component files.
From what I can tell, the webpack config was originally introduced in #318 to add support for a '@' import alias, which support for that alias was then removed in #435. What's left seems to be completely supported in the typescript preset, unless I am completely missing something here please let me know.

Also notice that support for .mdx files is very easily added by adding it to the stories blob config above. With that in place, you can add a basic sample MDX file to the template like:

import { Meta, Story, Preview } from '@storybook/addon-docs/blocks';
import { Thing } from '../src';

<Meta title="Thing" component={Thing} />

# Thing

With `MDX` we can define a story for `Thing` right in the middle of our
markdown documentation.

<Preview>
  <Story name="Thing MDX">
    <Thing />
  </Story>
</Preview>

Who does this impact? Who is this for?

All users looking to use this library to create a React component library with storybook and typescript.

Describe alternatives you've considered

Alternatively maybe adding to the documentation would help.

Additional context

Open to making a PR (or if someone on the team wants to make the quick change that would be cool too), just wanted to make sure this makes sense to folks and that I'm not completely missing something. Let me know what you guys think, thanks.

@kylemh
Copy link
Contributor

kylemh commented Jan 22, 2020

I tried using @storybook/preset-typescript and it didn't actually work for me in a personal project. There are actually a lot of open issues in the Storybook repo regarding it.

MDX may be cool, but again - tried that in a personal project and faced tons of issues... I think there is just so much going on in 5.3 it's just gonna be awhile before everything is working together.

@swyxio
Copy link
Collaborator

swyxio commented Jan 22, 2020

agreed. also had issues with MDX and 5.3. i think @shilman is aware of these things and just needs time to figure it out.

@mariusespejo
Copy link
Author

Okay. Up to guys if you want to keep this ticket open or not.

If it helps:
I just tried this on a fresh tsdx project (selecting the storybook template) then changing the config to be what's above and it works great. But can understand if folks aren't seeing the same.

Using these versions (some are required by the preset):

"@storybook/addon-docs": "^5.3.8",
"@storybook/preset-typescript": "^1.2.0",
"react-docgen-typescript-loader": "^3.6.0",
"ts-loader": "^6.2.1",
"fork-ts-checker-webpack-plugin": "^4.0.2",

@kylemh
Copy link
Contributor

kylemh commented Jan 22, 2020

I think it's fair to keep open! I like both ideas, I just wouldn't bother trying them right now. I'm keeping my ears relatively close to Storybook. I'll update if I see things change on my end.

Also, if you can get it working feel free! I would just love to see it deployed first with the config as you're sharing.

@swyxio
Copy link
Collaborator

swyxio commented Jan 23, 2020

@mariusespejo maybe post up a repo for kyle if you can share it

@mariusespejo
Copy link
Author

Sure, here's a minimal example with the config proposed above @kylemh

I documented the steps I took to convert to that config in the README if that helps, or see my commits.

Running demo of an example MDX story with blocks for preview and props: Click here

@kylemh
Copy link
Contributor

kylemh commented Jan 23, 2020

Thanks a ton! Ah, I see where I went wrong...

At this point in the docs, we're not told that include is required, but the default is undefined. I simply tried getting the preset to work just passing the name of the preset as a string to the presets array (as advertised above the "advanced usage" section). Weird that they don't provide a better default for include.

Awesome, yeah feel free to at least PR for the preset change. Sounds like we should hear back from Jared regarding using MDX.

@shilman
Copy link

shilman commented Jan 23, 2020

Thanks guys. I merged a bad PR in preset-typescript in December and will back out that change. I spent 4h beating my head against the preset yesterday and almost rage-quit programming a few times during that ordeal.

Over the past year we've migrated most of Storybook's packages and public APIs to typescript, but we haven't done much to fix its DX for Typescript users. preset-typescript is a step in that direction (albeit a flawed one), and I recently created a typescript template for sb init that generates .tsx stories for CRA in typescript mode.

Since this is a community for TypeScript DX, I would be INCREDIBLY, OVERWHELMINGLY grateful if anybody wanted to team up to get Storybook's TypeScript experience over the line. We're in striking distance and this would be super high leverage work!

@mariusespejo
Copy link
Author

fyi, wanted to post an update on this:

@storybook/preset-typescript recently had a major version bump and react-docgen-typescript-loader is no longer bundled inside it. Read more about it here.

If I understood it correctly, it sounds like starting in Storybook v6 @storybook/react will automatically include the docgen functionality that was removed in the preset-typescript

The v2 preset-typescript doesn't seem to play well with the base tsconfig.json that tsdx provides as it is currently.

  • For example it complains that files inside the test directory are outside of the now recommended rootDir: './src' You have to update tsconfig to instead use rootDirs: ['./src', './test'] (although I'm not positive if that has any impact on the rest of tsdx)

For those that run into this ticket basically if you want to use @storybook/preset-typescript v2 today:

  • looks like you no longer need additional config like I had in my examples above,
  • you will need to keep the custom webpack config in the short-term for the react-docgen-typescript-loader to get prop tables to show up
  • update tsconfig like mentioned above
  • also notice that it seems like current docs now recommend adding the preset to the addons array instead which is confusing but I didn't look into it any deeper
module.exports = {
  stories: ['../stories/**/*.stories.(ts|tsx)'],
  addons: ['@storybook/preset-typescript'],  // add any other addon you use
  webpackFinal: async (config) => {  // remove once you update to Storybook v6 when it's released
    config.module.rules.push({
      test: /\.(ts|tsx)$/,
      use: [
        {
          loader: require.resolve('react-docgen-typescript-loader'),
        },
      ],
    });

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

    return config;
  },
};

For everyone else, the original webpack config from @kylemh still works great, no additional changes needed.

My recommendation for this ticket is to wait for Storybook v6 and I believe at that point you can just use the typescript-preset without any additional config like so:

module.exports = {
  stories: ['../stories/**/*.stories.(ts|tsx)'],
  addons: ['@storybook/preset-typescript'],  // add any other addon you use
};

Will try to confirm that once it's released.

@kylemh
Copy link
Contributor

kylemh commented Mar 20, 2020

I'll be integrating TSDX into a work project this weekend, so I'll be fixing the template up to match current recommendations if I can get it working. I agree waiting for Storybook 6 will likely be the best course.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 22, 2020

Will the new @storybook/preset-typescript also support MDX? It wasn't included in the stories config.

  • For example it complains that files inside the test directory are outside of the now recommended rootDir: './src' You have to update tsconfig to instead use rootDirs: ['./src', './test'] (although I'm not positive if that has any impact on the rest of tsdx)

As far as I understand, rootDirs is used for module resolution of relative paths, whereas rootDir just controls the output structure (which is why we needed to change the default, so code would get generated in dist/ and not dist/src/).
Changing it might affect some relative module resolution if you have some similarly named paths or something.

EDIT: this was actually a bug in the template include; test was removed from the include in v0.13.1.

@kylemh
Copy link
Contributor

kylemh commented Mar 22, 2020

It won't. I think we should leave that to somebody that uses MDX to integrate with personally.

@RickeyWard
Copy link

I dropped in this change and it worked first try no issue on a new new tsdx react-storybook project. Regardless if this get's added, would be nice in to toss it in the docs, because I was having trouble making this work until I found this.

@kylemh
Copy link
Contributor

kylemh commented Mar 29, 2020

I'm opening a PR today to update the preset 💃

Edit: I dove into this a bunch and chatted a bunch with Michael. Unfortunately, react-docgen has quite a ways to go until it's remotely as good as the old setup. I see the bugs the switch has resolved, but there are significant features taken away.

I'll simply update the latest v2 preset and add an MDX example and consider the update done for now.

@shilman
Copy link

shilman commented Mar 30, 2020

Thanks for the updates everybody. At Storybook we're doing our best to nail this thing, but flying half blind. All input welcome to try to get this working smoothly.

Re: MDX I'm not sure what it means to support MDX. AFAIK MDX itself doesn't support TS, so you should be able to write your components in TS and use them in TS, but JS only inside the MDX file.

@mariusespejo
Copy link
Author

hey folks just wanted to provide an update here before I mark this closed.

The conversation here is no longer relevant with the release of Storybook v6 which looks like has direct support for typescript. Sounds like preset-typescript is no longer recommended.

However the storybook template config does still need to be updated which looks like someone already took the effort to determine the next best minimal config on PR #805 (which I tested myself, works great with docs addon and is able to automatically do the docgen stuff behind the scenes, allowing for prop tables to automatically show, etc.).

Anyways I'm not familiar with the PR process for this project but I would suggest reviewing and merging #805 to resolve the original intent of this ticket (to rely on Storybook's own recommended way to configure TS and stick to the intention of this project of minimizing the config).

@agilgur5 agilgur5 linked a pull request Aug 19, 2020 that will close this issue
@agilgur5 agilgur5 added solution: outdated This is not up-to-date with the current version templates scope: templates Related to an init template, not necessarily to core (but could influence core) labels Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: templates Related to an init template, not necessarily to core (but could influence core) solution: outdated This is not up-to-date with the current version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants