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 addon storyshots to TS #7674

Merged
merged 25 commits into from
Oct 8, 2019
Merged

Conversation

gaetanmaisse
Copy link
Member

What I did

  • Migrate @storybook/addon-storyshots and @storybook/addon-storyshots-puppeteer to TypeScript
  • Add types and classes to have a simpler DX when working on this addon
  • Refactor import/export to use named exports instead of default ones

Tracking Issue: #5030

@vercel
Copy link

vercel bot commented Aug 4, 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-ts-migration-addon-storyshots.storybook.now.sh

@gaetanmaisse
Copy link
Member Author

@shilman @ndelangen should all stories from storyshots-core be migrated to TS? kept to JS? Duplicated to TS?
I also read a bit about CSF format, maybe they should be converted to it?

@vercel vercel bot temporarily deployed to staging August 8, 2019 20:07 Inactive
@gaetanmaisse gaetanmaisse force-pushed the ts-migration/addon-storyshots branch from c28cccb to 9fef497 Compare August 8, 2019 20:12
@vercel vercel bot temporarily deployed to staging August 8, 2019 20:12 Inactive
storyKindRegex?: RegExp | string;
storyNameRegex?: RegExp | string;
framework?: SupportedFramework;
test?: (story: any, context: any, renderTree: RenderTree, options?: any) => any;
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 @kroeder are there already existing types somewhere for story and context?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in lib/addons

@@ -26,6 +26,9 @@
},
"dependencies": {
"@storybook/addons": "5.2.0-beta.22",
"@types/glob": "^7.1.1",
"@types/jest": "^24.0.16",
"@types/jest-specific-snapshot": "^0.5.3",
Copy link
Member Author

Choose a reason for hiding this comment

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

@emilio-martinez @kroeder @ndelangen what's the best practice about @types in SB codebase? I added them in dependencies because some of the types I exported in this addon are based on jest (and other libs) ones. So if a SB user want to use SB type it should install @types/jest too... 🤷‍♂

Copy link
Member

Choose a reason for hiding this comment

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

We'll talk about this later today, but I think the preliminary agreement is to add the types of a regular dependency also as a regular dependency.

@@ -42,7 +44,9 @@ function load(options) {
};
}

export default {
const angularLoader: Loader = {
Copy link
Member Author

Choose a reason for hiding this comment

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

It's the way I found to have type-check according to Loader class, if anyone has a better proposition?

…don-storyshots

Also improve a bit the typings
Make react native folder name and so loader name consistent with others and also with storyshot "framework" option (that support 'react-native' string)
`puppeteer-core` package is a version of `puppeteer` that doesn't download Chromium by default.
It looks like chromium is not directly used here as it was only a `peerDependencies` or an `optionalDependencies`.
So I moved to `puppeteer-core` to be able to compile TS sources of this addon without downloading Chromium at every `yarn install`
@gaetanmaisse gaetanmaisse force-pushed the ts-migration/addon-storyshots branch from 42d45a0 to aad29ed Compare October 8, 2019 06:13
@gaetanmaisse gaetanmaisse merged commit 38d8c4f into next Oct 8, 2019
@gaetanmaisse gaetanmaisse deleted the ts-migration/addon-storyshots branch October 8, 2019 06:39
elibarzilay pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jan 15, 2020
* [@storybook/addon-storyshots] remove typings as they're now provided by the package directly

TS migrate of the package was done in storybookjs/storybook#7674

* [@storybook/addon-storyshots-puppeteer] remove typings as they're now provided by the package directly

TS migrate of the package was done in storybookjs/storybook#7674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants