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

Migrate @storybook/html to Typescript #7338

Merged

Conversation

emilio-martinez
Copy link
Contributor

Issue: #5030

What I did

Migrate @storybook/html to Typescript

How to test

Everything should function the same, except the package should now have type definitions.

Note: The first commit is intentionally mostly a rename from js => ts to make the review easier via the subsequent commits.

@vercel
Copy link

vercel bot commented Jul 8, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-emilio-martinez-ts-migration-storybook-html.storybook.now.sh

@@ -1,4 +1,7 @@
export function webpack(config) {
// eslint-disable-next-line import/no-extraneous-dependencies
import { Configuration } from 'webpack';
Copy link
Member

Choose a reason for hiding this comment

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

I don't know well how this part of SB is working but maybe webpack should be a peerDependency?
@ndelangen can surely help us with this ;)

Copy link
Member

Choose a reason for hiding this comment

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

You can create a dependency on webpack for now, in monoconfig, I'll try an consolidate these types into a coherent whole.

When you add the dependency, be sure to add a wide version range.

Copy link
Member

@Hypnosphi Hypnosphi Jul 8, 2019

Choose a reason for hiding this comment

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

I think we should declare all the preset function types in @storybook/core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify, in this case Configuration is being picked up through @types/webpack which is a devDependency at the root of the repo. Technically, there's no runtime use or type definitions coming from webpack here.

It would make sense to add @types/webpack as a devDependency here—I could match the version at the root of the repo to avoid any conflicts. Given the above, do you guys still feel I should add webpack as a dependency? Let me know your thoughts 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gaetanmaisse @ndelangen let me know your thoughts on my last comment and I'll make the change :)

@ndelangen
Copy link
Member

I recently did some work on app/react & app/angular

Those are now typescript as well. Would be great if this PR would follow those examples.

@emilio-martinez
Copy link
Contributor Author

@ndelangen latest commit is up for you to take a look—rebased against next and aligned the types to app/react & app/angular.

@ndelangen ndelangen added this to the 5.2.0 milestone Jul 11, 2019
@ndelangen ndelangen merged commit 3876f2f into storybookjs:next Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
html maintenance User-facing maintenance tasks typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants