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

Clarify main.ts typescript instructions #18101

Merged
merged 6 commits into from
Jul 6, 2022
Merged

Clarify main.ts typescript instructions #18101

merged 6 commits into from
Jul 6, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Apr 29, 2022

Issue:

What I did

I was having a hard time understanding the documentation for configuring a .storybook/main.ts file, and I noticed that there was no mention of the vite builder, which has slightly different types, and I've heard that the vite builder is intended to be included in official documentation, so I added a few notes.

I still have some questions though, like whether or not this statement is true:

This babel config will only be used to process files in the `.storybook/` directory, and will not effect your stories.

I'll be honest, I dislike having to add this file. It's going to be confusing for vite users, who are not accustomed to dealing with babel config. And, will I need to add those babel packages to my project, or can I assume they're installed with storybook?

Why is adding the babel config necessary? Can't storybook automatically use the necessary config when processing the config files?

It sounds like adding ts-node is another way to solve this issue, should we mention that here for anyone who's reluctant to add a babel config?

How to test

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

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Apr 29, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a5eace1. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

docs/configure/overview.md Outdated Show resolved Hide resolved
Co-authored-by: Michael Shilman <[email protected]>
@IanVS
Copy link
Member Author

IanVS commented May 2, 2022

After discussing, turns out that the babel config will be used in your stories, so I'll need to change that wording.

Also the babel config itself isn't great, it shouldn't contain preset-env.

We can also suggest adding ts-node as another way to solve this issue. And maybe we'll investigate in the future using esrun instead.

I'll make these edits and re-request a review.

@jonniebigodes
Copy link
Contributor

@IanVS, thanks for taking the time and help us improve the documentation and provide additional information on the fantastic work you've been doing with Vite. Let me know once the changes are up, and I'll take a look and follow up with you should any changes need to happen. Sounds good?

@eric-burel
Copy link
Contributor

eric-burel commented May 3, 2022

It would be interesting to add a word on ts-node: whether it is installed or not greatly changes Storybook behaviour, namely it doesn't seem to respect the local tsconfig.
It seems to be an indirect dependency of interpret:

const moduleDescriptor = interpret.extensions[candidateExt];

This is all the more confusing in a monorepo setup, as ts-node could be installed at the root and not even in the current package/app where storybook is setup.

I could'nt really confirm how much the behaviour changes though, and also this might actually be considered a bug.

It may specifically trip people who set up a custom server in Next.js, because it's common to install ts-node in this scenario.

@Hideman85
Copy link

Hi, I think there is a second point that is not clear and causing me an issue, the support of ESM.
There are plenty of issues opened about this and I havent found a working solution.
I'm using react, storybook, ts and vite, I have my main.ts and my package.json type: module.

I dont know if you have experience with this too, but that would be awesome to have a piece of documentation on this to have a working setup.

I'm eager to help there if I can since like multiple other I'm really struggling with this, the vite app works like a charm, but storybook complains whatever I try to do 🙁

@IanVS
Copy link
Member Author

IanVS commented May 25, 2022

@Hideman85 one of the reasons I haven't come back to finish this up is that I myself am also not able to get things working in my own app, using the same config as you. I think that until Storybook makes some core changes in the way that it loads user modules to support ESM, we're a bit out of luck. :(

@ndelangen
Copy link
Member

@IanVS PLease have a look at future/base if you haven't already.

I updated all the main.js files in all examples to main.ts and added proper types there.
Perhaps we could rebase this unto future/base and use those main.ts files as examples?

# Conflicts:
#	docs/configure/overview.md
@ndelangen ndelangen merged commit 4dd5872 into next Jul 6, 2022
@ndelangen ndelangen deleted the typescript-config branch July 6, 2022 18:15
@IanVS
Copy link
Member Author

IanVS commented Jul 6, 2022

Hmmm, I maybe should have added a "do not merge" label. Some of the documentation here was incorrect:

This babel config will only be used to process files in the `.storybook/` directory, and will not affect your stories.

As I learned later, that's incorrect.

@ndelangen
Copy link
Member

Do I have to revert?

@IanVS
Copy link
Member Author

IanVS commented Jul 7, 2022

I'll push up another PR to fix it forward, how's that sound? A few changes in this PR are worth keeping, I think.

Edit: I've opened #18649.

@ndelangen
Copy link
Member

Merged!

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.

6 participants