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

React: Load root tsconfig.json into docgen-typescript if none provided #11184

Merged
merged 2 commits into from
Jun 17, 2020
Merged

React: Load root tsconfig.json into docgen-typescript if none provided #11184

merged 2 commits into from
Jun 17, 2020

Conversation

hipstersmoothie
Copy link
Contributor

Issue: #11146

What I did

Use root tsconfig.json for react-docgen-typescript if no tsconfig options are provided

How to test

  • Is this testable with Jest or Chromatic screenshots? no
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? no

Tested in bug reproduction repo

Screen Shot 2020-06-15 at 6 28 19 PM

@shilman shilman changed the title load root tsconfig.json into docgen-typescript if none provided React: Load root tsconfig.json into docgen-typescript if none provided Jun 16, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM @hipstersmoothie ! Thanks for the quick fix!!!

@mrmckeb can you review? we probably need something similar for the CRA preset?
@ndelangen we add path.resolve in other parts of the code. do we need it here?

@shilman
Copy link
Member

shilman commented Jun 16, 2020

@hipstersmoothie unrelated to this PR. this snapshot is inconsistent and toggles back and forth between the two orders randomly: https://www.chromatic.com/snapshot?appId=5a375b97f4b14f0020b0cda3&id=5ee824c77183c0002207abf8

Is there a good fix in react-docgen-typescript-plugin or react-docgen-typescript to make this consistent?

@hipstersmoothie
Copy link
Contributor Author

I’ll take a look tomorrow!

export function webpackFinal(config: Configuration, { typescriptOptions }: StorybookOptions) {
const { reactDocgen, reactDocgenTypescriptOptions } = typescriptOptions;
const { reactDocgen, reactDocgenTypescriptOptions = {} } = typescriptOptions;
Copy link
Member

Choose a reason for hiding this comment

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

This is already the default, it's set in defaults.js :)
https://github.com/storybookjs/storybook/blob/next/lib/core/src/server/config/defaults.js

We added it very recently. However, if it's not working for you - let me know, as that would be a bug.

!reactDocgenTypescriptOptions.compilerOptions &&
fs.existsSync(tsconfigPath)
) {
reactDocgenTypescriptOptions.tsconfigPath = tsconfigPath;
Copy link
Member

Choose a reason for hiding this comment

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

Could this be something that's done internally, inside the plugin? As a user, I prefer tools to default to the standard location/name for a tsconfig.json if I don't provide one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

@shilman shilman added this to the 6.0 typescript milestone Jun 16, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM except one minor question

@@ -47,7 +47,7 @@
"enzyme-adapter-react-16": "^1.9.1",
"enzyme-to-json": "^3.4.1",
"fork-ts-checker-webpack-plugin": "^4.0.3",
"react-docgen-typescript-plugin": "^0.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this dep at all? i made RDTP a hard dep in the CRA preset because it was breaking with SB 5.3 instances.

Copy link
Member

Choose a reason for hiding this comment

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

And it's already a dep in @storybook/react

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.

3 participants