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 monorepo to strict TS #22176

Closed
kasperpeulen opened this issue Apr 20, 2023 · 63 comments · Fixed by #24069
Closed

Migrate monorepo to strict TS #22176

kasperpeulen opened this issue Apr 20, 2023 · 63 comments · Fixed by #24069
Assignees
Labels
build Internal-facing build tooling & test updates good first issue help wanted maintenance User-facing maintenance tasks

Comments

@kasperpeulen
Copy link
Contributor

kasperpeulen commented Apr 20, 2023

What

We want to step our TS game in the monorepo and enable strict typescript in all packages!

Why

Having TS track for you if a variable might be null or not, enables us to code with much more confidence,
and also gives us quick in editor feedback, when you make assumptions that are not actually true!

How

Feel free to contribute to any of packages in the list below!

After you cloned the repo, you can best run this command:
yarn task --task check --no-link --start-from=install

This will install all dependencies, compile it (including dts files, that is what no-link does), and then check typescript errors in all packages.

To enable strict mode you will need:

  1. Change in tsconfig.json "strict": false -> "strict": true
  2. Run yarn check in the directory of the package.
  3. Fix all TS errors.

We would like to change as little as possible of the actual runtime behavior in this migration.
However, we also don't want to simply silence the compiler in every error with !, as or ts-ignore to get this migration in.
As a rule of thumb, if the logic is easy enough, prefer improving the code (e.g. add a null check, throw when not null, possibly use https://github.com/alexreardon/tiny-invariant) over silencing the compiler.
If the change needed to do the right thing, is too risky, and not in your expertise, it is okay to silence the compiler.
It is not ideal, but we still gain the benefit that new code written will have extra typesafety.

package name assign to merged PR
@storybook/addon-backgrounds @kasperpeulen #22178
@storybook/addon-docs @kasperpeulen #22180
@storybook/addon-highlight @kasperpeulen #22181
@storybook/addon-interactions @kasperpeulen
@storybook/addon-jest @Gufah #22389
@storybook/addon-mdx-gfm @1234tgk #22659
@storybook/addon-measure @efrenaragon96 #22402
@storybook/addon-outline @kyletsang #22369
@storybook/addon-storyshots @1234tgk #22487
@storybook/addon-storyshots-puppeteer @1234tgk #22407
@storybook/addon-storysource @1234tgk #22367
@storybook/addon-viewport @1234tgk #22339
@storybook/addons @Beadko
@storybook/angular @1234tgk
@storybook/api
@storybook/blocks @ssingh3856
@storybook/channel-postmessage @EDuToit #22335
@storybook/channel-websocket @EDuToit #22364
@storybook/channels @EDuToit #22365
@storybook/cli @0916dhkim #22254
@storybook/client-api @efrenaragon96 #22421
@storybook/codemod
@storybook/components @Beadko #22569
@storybook/core-client @kolife01 #22447
@storybook/core-events @kolife01 #22447
@storybook/core-server @stilt0n #23182
@storybook/csf-tools @usrrname #22312
@storybook/docs-tools @efrenaragon96 #22567
@storybook/external-docs
@storybook/html-vite @usrrname #22293
@storybook/instrumenter @kyletsang #22370
@storybook/manager @Dobariya-Nishant
@storybook/manager-api @subhajit20
@storybook/postinstall @kuriacka #22200
@storybook/preact-vite @mariasimo
@storybook/preset-create-react-app @kuriacka
@storybook/preset-vue-webpack @kyletsang #22320
@storybook/preset-vue3-webpack @chakAs3 #23377
@storybook/react-vite @mariasimo #22428
@storybook/router @kuriacka #22200
@storybook/scripts
@storybook/server @unpunnyfuns
@storybook/source-loader @usrrname #22420
@storybook/svelte-vite @kolife01 #22411
@storybook/sveltekit @kolife01 #22412
@storybook/theming @kuriacka #22376
@storybook/types @kuriacka #22397
@storybook/vue3-vite @chakAs3 #23375
@storybook/vue3-webpack5 @chakAs3 #23376
@storybook/web-components @efrenaragon96 #22399
@storybook/web-components-vite @usrrname #22309

So if you'd like to help converting any of these ♥️ ! Please mention here, which ones, you can take on, and open a PR migrating a package.

If you need any help, please reach out to storybook maintainers on the contributing channel on the Storybook discord.

@kasperpeulen kasperpeulen added the build Internal-facing build tooling & test updates label Apr 20, 2023
@kasperpeulen kasperpeulen self-assigned this Apr 20, 2023
@vanessayuenn vanessayuenn added the maintenance User-facing maintenance tasks label Apr 20, 2023
@kasperpeulen kasperpeulen added maintenance User-facing maintenance tasks and removed maintenance User-facing maintenance tasks labels Apr 20, 2023
@kuriacka
Copy link
Contributor

Hi,
I would like to help. Some packages like postinstall or router don't require any TS changes (except changing tsconfig.json of course). Do you want to create separate PR for each package or can I merge them into one?

Thanks in advance for your reply.

@kasperpeulen
Copy link
Contributor Author

@kuriacka Sounds great, one PR or multiple are both fine!

Let me now if you start work on any, then I will put your in the assign list so that me (and others) won't do the same work!

@kuriacka
Copy link
Contributor

kuriacka commented Apr 21, 2023

@kasperpeulen Thanks for info, I can start with aforementioned postinstall and router, then I can pick another packages.

@kuriacka
Copy link
Contributor

kuriacka commented Apr 21, 2023

@kasperpeulen Here is the PR for postinstall and router.
I can take another packages, please assign to me @storybook/theming and @storybook/preset-create-react-app.

Thanks!

@mariasimo
Copy link
Contributor

Hey @kasperpeulen! I'd like to help too, but I'm not very familiar yet with all the Storybook packages. Which one do you think can be a good one to start?

Thanks!

@suiux
Copy link

suiux commented Apr 23, 2023

Hi @kasperpeulen! I'd like to help too!
I don't have much experience with the Storybook packages yet. Could you recommend a good one for me to begin with?
Thanks!

@kasperpeulen
Copy link
Contributor Author

@mariasimo @suiux I think the packages in the frameworks directory are probably the easiest!
So @storybook/web-components-vite, @storybook/react-vite, @storybook/html-vite, @storybook/preact-vite

@mariasimo
Copy link
Contributor

@kasperpeulen thanks! I can take @storybook/react-vite and @storybook/preact-vite to begin with, if that's ok!

@kasperpeulen
Copy link
Contributor Author

Sounds great @mariasimo, I assigned them to you :)

@shilman
Copy link
Member

shilman commented Apr 25, 2023

w00t!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.1.0-alpha.9 containing PR #22181 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb@next upgrade --tag future

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Apr 25, 2023
@0916dhkim
Copy link
Contributor

0916dhkim commented Apr 26, 2023

Wait, aren't there a lot more packages that aren't migrated to strict mode yet? I wanted to contribute to @storybook/cli

@kasperpeulen
Copy link
Contributor Author

@0916dhkim Reopened!

@joe-vaughan
Copy link

Apologies! An automation went rogue 😅

Also, for anyone contributing to this or interested in taking part: we've just set up a new Discord working group for the project! Share updates on how you're getting, ask any questions, or simply chat with contributors 🎀

https://discord.com/channels/486522875931656193/1100689736022106173

@kasperpeulen kasperpeulen pinned this issue Apr 26, 2023
@usrrname
Copy link
Contributor

I'll take @storybook/html-vite if no one's on it already

@unpunnyfuns
Copy link

I'm looking very intently at @storybook/server.

@Beadko
Copy link

Beadko commented May 16, 2023

PR for the storybook/components #22569

@Beadko
Copy link

Beadko commented May 19, 2023

May I pick up the @storybook/addons if it's still free?

@ssingh3856
Copy link

Is @storybook/blocks still open or is it taken up by anybody else? If not can i work on it?

@1234tgk
Copy link
Contributor

1234tgk commented May 21, 2023

@storybook/addons-mdx-gfm is finished. I just had to switch strict: false to strict: true #22659

@storybook/addons-outline looks like it already has strict TS. Let me know if that is not the case.

May I take @storybook/angular if no one took it, please?

@shilman
Copy link
Member

shilman commented Jun 4, 2023

ZOMG!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.1.0-alpha.27 containing PR #22399 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb@next upgrade --tag future

@mateoguzmana
Copy link

mateoguzmana commented Jun 8, 2023

I'm looking into @storybook/manager-api (PR: #22973) and @storybook/manager as I see those are still free

@cupton15
Copy link

Could I take @storybook/codemod?

@stilt0n
Copy link
Contributor

stilt0n commented Jun 21, 2023

I've started working on @storybook/core-server since it appears to be free.

@aditya1
Copy link
Contributor

aditya1 commented Jul 8, 2023

I would like to help with @storybook/codemod

@Dobariya-Nishant
Copy link

i want to help in @storybook/manager

@cupton15
Copy link

Ok looks like I didn't finish @storybook/codemod in time, I'll pick up @storybook/core-server

@stilt0n
Copy link
Contributor

stilt0n commented Jul 21, 2023

Hi @cupton15 - I already have a PR open for core-server.

@mastrzyz
Copy link
Contributor

Trying to help out with some of the post migration cleanup ->

#23931
#23932

@LuisChiej
Copy link

Hello @kasperpeulen, I'm trying to migrate @storybook/external-docs but I keep running into errors running yarn check
Screenshot 2023-08-23 at 11 50 42
On running yarn install, I get this
Screenshot 2023-08-23 at 11 43 44

@kasperpeulen
Copy link
Contributor Author

@LuisChiej I think we can actually skip external docs. I think this is a package that we likely want to remove in the feature, maybe @shilman has more context about that.

@subhajit20
Copy link
Contributor

@kasperpeulen hey I would like to help. Following @storybook/manager-api is still free. can you assign it to me ?

@kasperpeulen
Copy link
Contributor Author

@subhajit20 Done!

@johnhunter
Copy link
Contributor

Hey, I'd like to help out with this work. Of the remaining unassigned packages it looks like codemod and scripts are the ones left to do:

  • @storybook/api (deprecated, see @storybook/manager-api)
  • @storybook/external-docs (going to be deprecated)
  • @storybook/codemod
  • @storybook/scripts

I can look at either, @kasperpeulen any preference?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Internal-facing build tooling & test updates good first issue help wanted maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging a pull request may close this issue.