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

Core: Zero-config TypeScript loading #10813

Merged
merged 42 commits into from
May 21, 2020
Merged

Core: Zero-config TypeScript loading #10813

merged 42 commits into from
May 21, 2020

Conversation

mrmckeb
Copy link
Member

@mrmckeb mrmckeb commented May 17, 2020

Issue: #10790 #4475

What I did

Added support for TypeScript to the core of Storybook.

Vue support based on: storybookjs/presets#122.

How to test

Projects should work with TypeScript with zero config.

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2020

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 7a4724b

@shilman shilman changed the title feat: add root TypeScript support Core: Add zero-config TypeScript support May 17, 2020
@mrmckeb mrmckeb force-pushed the mrmckeb/issue10790 branch 2 times, most recently from 0d7b7fe to c918123 Compare May 17, 2020 12:01
@yannbf
Copy link
Member

yannbf commented May 17, 2020

This is awesome stuff! 🙌
Let me know if I can assist on testing or something! :)

@shilman shilman changed the title Core: Add zero-config TypeScript support Core: Zero-config TypeScript support May 18, 2020
@graup
Copy link
Contributor

graup commented May 18, 2020

I added a component to the vue-kitchensink using typescript (pushed to your branch on @shilman's request). Seems to work fine 👍

I'm also testing this with a vue-cli+babel+ts project of mine and running into some problems there, will do some more tests and report back later today!

@shilman
Copy link
Member

shilman commented May 20, 2020

Problem

There seems to be a performance regression that's causing cypress to fail.

Some numbers from a month ago:

  • preview (generated-entry): 470ms
    - pushTomanager: 200ms
  • manager: 1100ms
    - telejson parse: 150ms
    - setStories: 900ms

Next (today)

  • preview (generated-entry): 540ms
    - pushTomanager: 250ms
  • manager: 1250ms
    - telejson parse: 120ms
    - setStories: 870ms

This branch

  • preview (generated-entry): 2300ms
    - pushTomanager: 1900ms
  • manager: 2700ms
    - telejson parse: 1200s
    - setStories: 1300ms

Solution

There are a couple different issues here.

  1. Args is serializing a lot more data over the channel than before
  2. react-docgen-typescript is creating a lot more data than react-docgen due to imported types (e.g. styled components can have hundreds of ArgTypes)

Worked with @mrmckeb on a tentative solution, which is to exclude ArgTypes that come from node_modules by default. We'll document this as part of the release.

As a secondary issue, worked on #10373 with @tmeasday to reduce the data sent over the channel by "normalizing" story parameters (e.g. sending component-level parameters once per component rather than once per story). The benefits of that work are only being partially realized, and will follow up in a separate issue: #10844

@mrmckeb mrmckeb requested a review from usulpro as a code owner May 20, 2020 08:15
Comment on lines +5 to +6
export const createBabelLoader = (options, framework) => ({
test: useBaseTsSupport(framework) ? /\.(mjs|tsx?|jsx?)$/ : /\.(mjs|jsx?)$/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is a problem?

@mrmckeb mrmckeb requested a review from stijnkoopal as a code owner May 20, 2020 10:43
@shilman
Copy link
Member

shilman commented May 21, 2020

FYI I'm merging this with unaccepted Chromatic baselines because I want to get it out ASAP but also want to work through the changes one by one.

@ndelangen @mrmckeb @patricklafrance @tmeasday

@shilman shilman merged commit 7336aa4 into next May 21, 2020
*
* @example `['./src/*.stories.(j|t)sx?']`
*/
stories: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndelangen stories can be a function returning an array of strings right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's possible, we can add that @ndelangen @shilman :)

I'm hoping this is temporary @matheo, as this would be exported differently once we convert core to TS.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All preset properties can be a T | (T, Options) => T | (T, Options) => Promise<T>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndelangen That applies to addons and stories?

Specifically we're looking at the options here: https://github.com/storybookjs/storybook/blob/next/lib/core/types/index.ts#L8-L32

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addons & presets are the exception.
they are: string[].

everything else is T | (T, Options) => T | (T, Options) => Promise<T>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the technical reason is, as we "discover" presets/addons, we don't know the whole list yet, and so we can't pass it in. I figured it'd be too complex and confusing for presets to receive an incomplete list of presets.

@mrmckeb mrmckeb deleted the mrmckeb/issue10790 branch May 25, 2020 06:33
@RuneKR
Copy link

RuneKR commented Jun 30, 2020

This PR undo the work that have been done in previous pull requests to increase the performance of docgen and it breaks compatibility with "path alias" that worked in previous versions.

This plugin is the issue: react-docgen-typescript-loader #7998

https://github.com/storybookjs/presets/blob/32b0e861bbea7a0972d2bd99f5cb6a6851a85ce1/packages/preset-typescript/CHANGELOG.md

@shilman
Copy link
Member

shilman commented Jul 1, 2020

@RuneKR please read about the rationale for the change here:

https://github.com/storybookjs/storybook/blob/next/addons/docs/react/README.md#typescript-props-with-react-docgen

if you want to use the old settings, you can do that by setting typescript: { reactDocgen: 'react-docgen' } in your .storybook/main.js.

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

Successfully merging this pull request may close these issues.

None yet

9 participants