Increase type safety #89
Conversation
|
Hey @valentinpalkovic thank you so much for this contribution, I love this!! There are still a couple issues in this PR, could you check them? It happens when running yarn update-all
yarn run v1.22.17
$ yarn update-configs && yarn update-docs
$ ts-node ./tools/update-configs
/Users/yannbraga/open-source/eslint-plugin-storybook/node_modules/ts-node/src/index.ts:750
return new TSError(diagnosticText, diagnosticCodes);
^
TSError: ⨯ Unable to compile TypeScript:
tools/update-configs.ts:8:28 - error TS7016: Could not find a declaration file for module '../.prettierrc'. '/Users/yannbraga/open-source/eslint-plugin-storybook/.prettierrc.js' implicitly has an 'any' type.
8 import prettierConfig from '../.prettierrc'
~~~~~~~~~~~~~~~~
tools/update-configs.ts:10:16 - error TS2665: Invalid module name in augmentation. Module '../.prettierrc' resolves to an untyped module at '/Users/yannbraga/open-source/eslint-plugin-storybook/.prettierrc.js', which cannot be augmented.
10 declare module '../.prettierrc' {}
~~~~~~~~~~~~~~~~
tools/update-configs.ts:27:7 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ 'import/no-anonymous-default-export': string; }'.
No index signature with a parameter of type 'string' was found on type '{ 'import/no-anonymous-default-export': string; }'.
27 setting[rule.ruleId] = rule.meta.docs.recommended || 'error'
~~~~~~~~~~~~~~~~~~~~
tools/update-configs.ts:43:29 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ csf: null; recommended: null; 'csf-strict': string; }'.
No index signature with a parameter of type 'string' was found on type '{ csf: null; recommended: null; 'csf-strict': string; }'.
43 const extendsCategoryId = extendsCategories[category.categoryId] |
|
Hey @yannbf I will take care of it directly after The Easter holidays. :) Have a nice holiday time! |
Thank you so much! I wish you an incredible holiday!! |
valentinpalkovic
force-pushed
the
increase-type-safety
branch
from
4c1ac9e
to
514d50a
Compare
Added some types and removed obosolete any type assertions
It seems, that ts-node does a different kind of type checking than executing `tsc --noEmit`. While the first one outputs errors, the second one doesn't. Therefore, the type checking will be disabled while executing ts-node, but a further step in `yarn test:ci` is introduced to check the types explicitly.
valentinpalkovic
force-pushed
the
increase-type-safety
branch
from
514d50a
to
ec2f090
Compare
|
@yannbf Please take another look. It seems that |
There was a problem hiding this comment.
Thank you so much for making these changes @valentinpalkovic ! I added a couple more comments. Kinda interesting to know about ts-node, I think I might be experiencing a similar issue in another project for something completely different!
|
@yannbf Could you take another look? :) |
There was a problem hiding this comment.
Hey @valentinpalkovic sorry for the delay, I've been sick.
It all looks really good, thank you so much for improving this codebase! Now it's much better for new contributors and I'm sure they'll all appreciate your work. Cheers!
|
Hey @yannbf thank you for the review! If everything looks fine, should I merge it? :) I'm just asking because normally I haven't triggered any merges in storybookjs repos so far and I want to make sure if it is ok to do so. |
Sure, go ahead! |
|
🚀 PR was released in |
What Changed
Added some types and removed obsolete "any" type assertions by replacing them through stronger types
Change Type
Indicate the type of change your pull request is:
maintenancedocumentationpatchminormajor📦 Published PR as canary version:
0.5.11--canary.89.28723b8.0✨ Test out this PR locally via:
npm install eslint-plugin-storybook@0.5.11--canary.89.28723b8.0 # or yarn add eslint-plugin-storybook@0.5.11--canary.89.28723b8.0