-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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/components to TS #6095
Migrate @storybook/components to TS #6095
Conversation
Mind this PR: You may want to base on that, it's close to being merged. |
@gaetanmaisse how's this going? Do you need assistance? This would be an immense step forward for our typescript migration epic. |
@ndelangen I migrate codebase to TS and the only thing still in progress before I can put this PR in review is fixing snapshot testing: as TS compiler generates js sources that are not the same as Babel ones it makes tests failed (some functions are anonymous in TS build). For instance: Babel generated source for Scrollbar: var ScrollArea = function ScrollArea(_ref4) {
var children = _ref4.children,
vertical = _ref4.vertical,
horizontal = _ref4.horizontal,
props = _objectWithoutProperties(_ref4, ["children", "vertical", "horizontal"]);
return _react.default.createElement(_react.Fragment, null, _ref5, _react.default.createElement(Scroll, _extends({
vertical: vertical,
horizontal: horizontal
}, props), children));
};
exports.ScrollArea = ScrollArea; TS generated source for Scrollbar: exports.ScrollArea = function (_a) {
var children = _a.children, vertical = _a.vertical, horizontal = _a.horizontal, props = __rest(_a, ["children", "vertical", "horizontal"]);
return (react_1.default.createElement(react_1.Fragment, null,
react_1.default.createElement(theming_1.Global, { styles: ScrollAreaStyles_1.getScrollAreaStyles }),
react_1.default.createElement(Scroll, __assign({ vertical: vertical, horizontal: horizontal }, props), children)));
}; With Babel compiler |
Codecov Report
@@ Coverage Diff @@
## next #6095 +/- ##
==========================================
+ Coverage 38.31% 39.92% +1.61%
==========================================
Files 649 649
Lines 9851 10222 +371
Branches 388 571 +183
==========================================
+ Hits 3774 4081 +307
- Misses 6017 6040 +23
- Partials 60 101 +41
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## next #6095 +/- ##
=========================================
- Coverage 41.04% 40.8% -0.24%
=========================================
Files 613 613
Lines 8454 8496 +42
Branches 378 502 +124
=========================================
- Hits 3470 3467 -3
- Misses 4929 4937 +8
- Partials 55 92 +37
Continue to review full report at Codecov.
|
@@ -185,4 +187,4 @@ export const getScrollAreaStyles = theme => ({ | |||
overflowY: 'hidden', | |||
overflowX: 'scroll', | |||
}, | |||
}); | |||
} as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why where you in need of casting any
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was fighting with types from @emotion/core
but I found a solution ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! 🚀
Don't have time to dig in too detail but I think it is a great work for sure! I've attached couple comments and referenced some great articles. Thank you very much! |
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-fork-gaetanmaisse-ts-migratio.storybook.now.sh |
"extends": "../../tsconfig.json", | ||
"compilerOptions": { | ||
"rootDir": "./src", | ||
"types": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kroeder I had to use types
to includes types of some libraries as it's done like this in "parent" tsconfig.json
, is there a reason to do it instead of letting TS compiler use all types available in /node_modules/@types/
? based on understanding of @types, typeRoots and types of https://www.typescriptlang.org/docs/handbook/tsconfig-json.html
There are still a lot of `any`, I need to dive deep in `emotion` to better understand how it works in order to provide good typings.
It looks like TS compiler does not generate the same as babel one and so some code must be rewrite in order to "force" TS compiler to generate JS code that is compatible with SB codebase (and that match tests snapshots).
…iled code" This reverts commit 538ee0f as TS is now compiled using Babel instead of tsc
Codecov Report
@@ Coverage Diff @@
## next #6095 +/- ##
=========================================
- Coverage 41.04% 40.8% -0.24%
=========================================
Files 613 613
Lines 8454 8496 +42
Branches 378 502 +124
=========================================
- Hits 3470 3467 -3
- Misses 4929 4937 +8
- Partials 55 92 +37
Continue to review full report at Codecov.
|
@@ -28,7 +28,7 @@ export default { | |||
], | |||
}; | |||
|
|||
export const singleItem = () => <ActionBar actionItems={[{ title: 'Clear', onClick: action1 }]} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this space removal doesn't seem intentional. or is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't, will be back in #6500 😉
Issue: #5030
What I did
@storybook/components
to TypeScriptLint (Storybook)
jobTODO in another PR