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/components to TS #6095

Merged
merged 21 commits into from
Apr 8, 2019
Merged

Migrate @storybook/components to TS #6095

merged 21 commits into from
Apr 8, 2019

Conversation

gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Mar 14, 2019

Issue: #5030

What I did

  • 🏗 Migrate @storybook/components to TypeScript
  • 🔧 Add some types and interfaces and use them to type codebase
  • ⚔️ Fight with TS compiler about anonymous functions, see Migrate @storybook/components to TS #6095 (comment) and 5fa1be5
  • ♻️ Localy disable a md linter rule (as we need to do what is done) to (try to) fix Lint (Storybook) job

TODO in another PR

  • 🛠 Refactor some imports/exports
  • 📦 Use these typings in addons

@github-actions
Copy link
Contributor

github-actions bot commented Mar 14, 2019

Fails
🚫

PR is not labeled with one of: ["cleanup","doc-dependencies:update","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS against d8b0858

@ndelangen
Copy link
Member

Mind this PR:
#5402

You may want to base on that, it's close to being merged.

@ndelangen
Copy link
Member

@gaetanmaisse how's this going? Do you need assistance?

This would be an immense step forward for our typescript migration epic.

@gaetanmaisse
Copy link
Member Author

gaetanmaisse commented Mar 28, 2019

@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 ScrollArea tag name will be used in snapshots but with TS it will be a default Component tag.

microsoft/TypeScript#14127 🤔

@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #6095 into next will increase coverage by 1.61%.
The diff coverage is 88.82%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/components/src/tooltip/ListItem.stories.tsx 100% <ø> (ø)
lib/components/src/form/form.stories.tsx 100% <ø> (ø)
...rc/syntaxhighlighter/syntaxhighlighter.stories.tsx 100% <ø> (ø)
lib/components/src/Badge/Badge.stories.tsx 100% <ø> (ø)
lib/components/src/brand/StorybookIcon.stories.tsx 100% <ø> (ø)
lib/components/src/spaced/Spaced.stories.tsx 100% <ø> (ø)
...components/src/placeholder/placeholder.stories.tsx 100% <ø> (ø)
lib/components/src/tooltip/TooltipNote.stories.tsx 100% <ø> (ø)
...components/src/tooltip/TooltipLinkList.stories.tsx 100% <ø> (ø)
lib/components/src/icon/icons.tsx 100% <ø> (ø)
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b93015...fe5fc7d. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #6095 into next will decrease coverage by 0.23%.
The diff coverage is 86.59%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/components/src/tooltip/ListItem.stories.tsx 100% <ø> (ø)
lib/components/src/form/form.stories.tsx 100% <ø> (ø)
...rc/syntaxhighlighter/syntaxhighlighter.stories.tsx 100% <ø> (ø)
lib/components/src/Badge/Badge.stories.tsx 100% <ø> (ø)
lib/components/src/brand/StorybookIcon.stories.tsx 100% <ø> (ø)
lib/components/src/spaced/Spaced.stories.tsx 100% <ø> (ø)
lib/components/src/tooltip/TooltipNote.stories.tsx 100% <ø> (ø)
...components/src/tooltip/TooltipLinkList.stories.tsx 100% <ø> (ø)
lib/components/src/icon/icons.tsx 100% <ø> (ø)
lib/components/src/tooltip/Tooltip.stories.tsx 100% <ø> (ø)
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1ffaac...d8b0858. Read the comment docs.

@@ -185,4 +187,4 @@ export const getScrollAreaStyles = theme => ({
overflowY: 'hidden',
overflowX: 'scroll',
},
});
} as any);
Copy link
Member

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?

Copy link
Member Author

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 ;)

Copy link
Member

@kroeder kroeder left a comment

Choose a reason for hiding this comment

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

Good job! 🚀

lib/components/src/form/input/input.tsx Show resolved Hide resolved
lib/components/src/tabs/tabs.tsx Outdated Show resolved Hide resolved
lib/components/src/tooltip/Tooltip.tsx Show resolved Hide resolved
lib/components/src/tooltip/WithTooltip.tsx Show resolved Hide resolved
@leoyli
Copy link
Contributor

leoyli commented Apr 3, 2019

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!

@vercel
Copy link

vercel bot commented Apr 3, 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-gaetanmaisse-ts-migratio.storybook.now.sh

"extends": "../../tsconfig.json",
"compilerOptions": {
"rootDir": "./src",
"types": [
Copy link
Member Author

@gaetanmaisse gaetanmaisse Apr 3, 2019

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
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #6095 into next will decrease coverage by 0.23%.
The diff coverage is 86.59%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/components/src/tooltip/ListItem.stories.tsx 100% <ø> (ø)
lib/components/src/form/form.stories.tsx 100% <ø> (ø)
...rc/syntaxhighlighter/syntaxhighlighter.stories.tsx 100% <ø> (ø)
lib/components/src/Badge/Badge.stories.tsx 100% <ø> (ø)
lib/components/src/brand/StorybookIcon.stories.tsx 100% <ø> (ø)
lib/components/src/spaced/Spaced.stories.tsx 100% <ø> (ø)
lib/components/src/tooltip/TooltipNote.stories.tsx 100% <ø> (ø)
...components/src/tooltip/TooltipLinkList.stories.tsx 100% <ø> (ø)
lib/components/src/icon/icons.tsx 100% <ø> (ø)
lib/components/src/tooltip/Tooltip.stories.tsx 100% <ø> (ø)
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1ffaac...d8b0858. Read the comment docs.

@gaetanmaisse gaetanmaisse merged commit bf76760 into storybookjs:next Apr 8, 2019
@gaetanmaisse gaetanmaisse deleted the ts-migration/lib-components branch April 8, 2019 14:46
@shilman shilman added ui maintenance User-facing maintenance tasks labels Apr 9, 2019
@@ -28,7 +28,7 @@ export default {
],
};

export const singleItem = () => <ActionBar actionItems={[{ title: 'Clear', onClick: action1 }]} />;
Copy link
Contributor

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?

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 wasn't, will be back in #6500 😉

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.

7 participants