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

Convert packages/react storybook to typescript. Convert checkbox story to tsx #12000

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
33c1b71
feat: tsconfig and eslint to support typescript
mbarrer Jul 6, 2022
38dd38d
feat: convert main.js storybook to ts. Convert checkbox story to tsx
mbarrer Jul 6, 2022
ec89107
feat: fix linting errors
mbarrer Jul 13, 2022
1566a5a
chore(merge): merge branch main
mbarrer Jul 15, 2022
b79c8f1
chore: update yarn.lock
mbarrer Jul 15, 2022
d896f6a
Merge pull request #1 from mbarrer/ts-storybook
mbarrer Jul 15, 2022
18c8b76
fix: change tsconfig module to commonjs
mbarrer Jul 15, 2022
056660d
Merge pull request #2 from mbarrer/ts-storybook
mbarrer Jul 15, 2022
a10ab9a
Merge branch 'main' of github.com:carbon-design-system/carbon
mbarrer Aug 23, 2022
d034edd
refactor: add comments to props and typing for playground args
mbarrer Aug 23, 2022
760ef50
chore: add comment for ambient module
mbarrer Aug 23, 2022
1e2fb9a
chore: rebuild with yarn 3.2.1
mbarrer Aug 23, 2022
cb16af7
chore: remove commented out types
mbarrer Aug 23, 2022
551d6ac
Merge branch 'main' of github.com:carbon-design-system/carbon
mbarrer Aug 23, 2022
1af0750
chore: add missing eslint parser devDependancy
mbarrer Aug 23, 2022
3f69616
chore(yarn): reset yarn.lock and update yarn cache
tay1orjones Aug 24, 2022
43e3966
refactor: add StorybookConfig typing to config object
mbarrer Aug 26, 2022
0bffe0d
Merge branch 'main' of github.com:mbarrer/carbon
mbarrer Aug 26, 2022
d188dd6
chore: remove TODO
mbarrer Aug 26, 2022
871d4d1
refactor: add ts-node and update mini-css-extract-plugin
mbarrer Sep 2, 2022
b3a4665
chore: update node imports
mbarrer Sep 2, 2022
af3541b
chore: run yarn dedupe
mbarrer Sep 2, 2022
6270409
Merge branch 'main' of github.com:carbon-design-system/carbon
mbarrer Sep 2, 2022
c05f566
chore: review comments
mbarrer Sep 19, 2022
1bbdc25
chore: update @typescript/eslint-plugin
mbarrer Sep 19, 2022
51765ae
refactor: remove any type from main.ts
mbarrer Sep 19, 2022
a55f812
Merge branch 'main' of github.com:carbon-design-system/carbon
mbarrer Sep 19, 2022
cfef450
chore: run yarn dedupe
mbarrer Sep 19, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/eslint-config-carbon/plugins/react.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ module.exports = {
],
},
overrides: [
{
files: ['*.ts', '*.tsx'],
plugins: ['@typescript-eslint'],
extends: ['plugin:@typescript-eslint/recommended'],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming carbon is using the no-unused-vars JS rule, you'll want to turn that off in favor of the typescript-eslint one. Here's an example

// Sometimes we'll want to define a quick component in a story to use as a
// wrapper for a component we're documenting. For example:
//
Expand Down
1 change: 1 addition & 0 deletions examples/codesandbox-with-sass-compilation/src/App.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import React from 'react';
import { Button } from '@carbon/react';

function App() {
Expand Down
1 change: 1 addition & 0 deletions examples/codesandbox/src/App.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import React from 'react';
import { Button } from '@carbon/react';

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/react/.storybook/.babelrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,5 @@
'use strict';

module.exports = {
presets: ['babel-preset-carbon'],
presets: ['babel-preset-carbon', '@babel/preset-typescript'],
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,16 @@
* This source code is licensed under the Apache-2.0 license found in the
* LICENSE file in the root directory of this source tree.
*/
import fs from 'fs';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a good practice to put node: before using each node core modules/lib, etc.

example:

import fs from 'node:fs' fs Docs
import path from 'node:path' Path docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh cool I didnt know you could reference these like this. good to know!

import glob from 'fast-glob';
import MiniCssExtractPlugin from 'mini-css-extract-plugin';
import path from 'path';

'use strict';

const fs = require('fs');
const glob = require('fast-glob');
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
const path = require('path');

const stories = glob
const stories: string[] = glob
.sync(
[
'./Welcome/Welcome.stories.js',
'../src/**/*.stories.js',
'../src/**/*.stories.@(js|tsx|ts)',
'../src/**/*.stories.mdx',
'../src/**/next/*.stories.js',
'../src/**/next/**/*.stories.js',
Expand All @@ -30,7 +27,7 @@ const stories = glob
// Filters the stories by finding the paths that have a story file that ends
// in `-story.js` and checks to see if they also have a `.stories.js`,
// if so then defer to the `.stories.js`
.filter((match) => {
.filter((match: string) => {
const filepath = path.resolve(__dirname, match);
const basename = path.basename(match, '.js');
const denylist = new Set([
Expand Down Expand Up @@ -92,9 +89,10 @@ module.exports = {
},
framework: '@storybook/react',
stories,
webpack(config) {
const babelLoader = config.module.rules.find((rule) => {
return rule.use.some(({ loader }) => {
// TODO: Fix typings for this function
mbarrer marked this conversation as resolved.
Show resolved Hide resolved
webpack(config: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If statically typing this is desired, there are two options:

  1. Only use Configuration type from Webpack
import type { Configuration } from 'webpack';
...
webpack(config: Configuration) {
  1. Type the entire config using StorybookConfig
import type { StorybookConfig } from '@storybook/core-common';

const config: StorybookConfig = { /** */ }

module.exports = config;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep nice call, there was a few other changes needed in the body of the config to account for the union types used in the webpackFinal prop. The use of the typings also caught the fact that the webpack property should have actually been renamed to webpackFinal according to the storybook 6.5 docs

const babelLoader = config.module.rules.find((rule: any) => {
return rule.use.some(({ loader }: { loader: string }) => {
return loader.includes('babel-loader');
});
});
Expand Down
2 changes: 2 additions & 0 deletions packages/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
"@storybook/manager-webpack5": "^6.5.6",
"@storybook/react": "^6.5.6",
"@storybook/theming": "^6.5.6",
"@typescript-eslint/eslint-plugin": "^5.30.5",
"autoprefixer": "^10.4.0",
"babel-loader": "^8.2.3",
"babel-plugin-dev-expression": "^0.2.3",
Expand Down Expand Up @@ -116,6 +117,7 @@
"storybook-readme": "^5.0.9",
"stream-browserify": "^3.0.0",
"style-loader": "^3.3.1",
"typescript": "^4.7.4",
"webpack": "^5.65.0",
"webpack-dev-server": "^4.7.4"
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { FunctionComponent, HTMLAttributes } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright statement


declare const CheckboxSkeleton: FunctionComponent<
HTMLAttributes<HTMLDivElement>
>;

export default CheckboxSkeleton;
52 changes: 52 additions & 0 deletions packages/react/src/components/Checkbox/Checkbox.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import * as React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright statement

import { ForwardRefReturn, ReactInputAttr } from '../../typings/shared';

type ExcludedAttributes = 'id' | 'onChange' | 'onClick' | 'type';

export interface CheckboxProps
extends Omit<ReactInputAttr, ExcludedAttributes> {
/**
* Provide an `id` to uniquely identify the Checkbox input
*/
id: string;

/**
* Provide a label to provide a description of the Checkbox input that you are
* exposing to the user
*/
labelText: NonNullable<React.ReactNode>;

/**
* Specify whether the underlying input should be checked by default
*/
defaultChecked?: boolean;

/**
* Specify whether the Checkbox should be disabled
*/
disabled?: boolean;

/**
* Specify whether the label should be hidden, or not
*/
hideLabel?: boolean;

/**
* Specify whether the Checkbox is in an indeterminate state
*/
indeterminate?: boolean;

/**
* Provide an optional handler that is called when the internal state of
* Checkbox changes. This handler is called with event and state info.
* `(event, { checked, id }) => void`
*/
onChange?: (
evt: React.ChangeEvent<HTMLInputElement>,
data: { checked: boolean; id: string }
) => void;
}

declare const Checkbox: ForwardRefReturn<HTMLInputElement, CheckboxProps>;

export default Checkbox;
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
*/

import React from 'react';
import { default as Checkbox, CheckboxSkeleton } from './';
import { default as Checkbox, CheckboxSkeleton } from '.';
import { ComponentStory } from '@storybook/react';
import mdx from './Checkbox.mdx';
import { CheckboxProps } from './Checkbox';

const prefix = 'cds';

Expand All @@ -24,7 +26,7 @@ export default {
},
};

export const CheckboxStory = () => {
export const CheckboxStory: ComponentStory<typeof Checkbox> = () => {
return (
<fieldset className={`${prefix}--fieldset`}>
<legend className={`${prefix}--label`}>Group label</legend>
Expand All @@ -38,11 +40,22 @@ CheckboxStory.storyName = 'Checkbox';

export const Skeleton = () => <CheckboxSkeleton />;

export const Playground = (args) => (
export const Playground = ({
labelText,
...args
}: Omit<CheckboxProps, 'id'>) => (
<fieldset className={`${prefix}--fieldset`}>
<legend className={`${prefix}--label`}>Group label</legend>
<Checkbox labelText={`Checkbox label`} id="checkbox-label-1" {...args} />
<Checkbox labelText={`Checkbox label`} id="checkbox-label-2" {...args} />
<Checkbox
labelText={labelText ?? 'Checkbox label'}
id="checkbox-label-1"
{...args}
/>
<Checkbox
labelText={labelText ?? 'Checkbox label'}
id="checkbox-label-2"
{...args}
/>
</fieldset>
);

Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/components/Checkbox/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import Checkbox from './Checkbox';
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing copyright statement

import CheckboxSkeleton from './Checkbox.Skeleton';

export { Checkbox, CheckboxSkeleton };
export default Checkbox;
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable jest/no-standalone-expect */
Copy link
Contributor

Choose a reason for hiding this comment

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

Which part of this file is violating this rule? Should it instead be updated to conform to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not sure why this was added, removing does not seem to cause any warnings to show

/**
* Copyright IBM Corp. 2016, 2018
*
Expand Down
5 changes: 5 additions & 0 deletions packages/react/src/custom.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// Ambient module to allow the importing of .mdx files in storybook .tsx files
declare module '*.mdx' {
const content: any;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely aim for zero warnings. This should either be unknown or if working around the lack of explicit any is too onerous to devs (since this is a gradual conversion), perhaps we consider disabling the no-explicit-any rule to start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, i should have changed this to unknown

export default content;
}
104 changes: 104 additions & 0 deletions packages/react/src/typings/shared.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import * as React from 'react';

export type ReactAttr<T = HTMLElement> = React.HTMLAttributes<T>;
export type ReactAnchorAttr<T = HTMLAnchorElement> =
React.AnchorHTMLAttributes<T>;
export type ReactButtonAttr<T = HTMLButtonElement> =
React.ButtonHTMLAttributes<T>;
export type ReactDivAttr = ReactAttr<HTMLDivElement>;
export type ReactInputAttr<T = HTMLInputElement> = React.InputHTMLAttributes<T>;
export type ReactLabelAttr<T = HTMLLabelElement> = React.LabelHTMLAttributes<T>;
export type ReactLIAttr<T = HTMLLIElement> = React.LiHTMLAttributes<T>;

// export type ShapeOf<
mbarrer marked this conversation as resolved.
Show resolved Hide resolved
// B extends object,
// E extends object = { [key: string]: any }
// > = (E extends never ? {} : E) & B;
// export type Overwrite<T, U> = [T] extends [never] ? U : Omit<T, keyof U> & U;

export type VerticalDirection = 'bottom' | 'top';
export type HorizontalDirection = 'left' | 'right';
export type Direction = HorizontalDirection | VerticalDirection;
export type ListBoxBaseItemType = object | string;
export type TooltipAlignment = 'center' | 'end' | 'start';
export type TooltipPosition = Direction;
export type CarbonSize = 'lg' | 'sm' | 'xs';
export type CarbonInputSize = 'sm' | 'lg' | 'xl';

//
// In retrospect, it may not always be a good idea to lump shared props into a common reused interface.
// There's no real relation between components that share these types and they could diverge causing painful refactors.
// This approach should probably be left for more complicated types such as those that involve generics.
//

export interface DownshiftTypedProps<ItemType> {
itemToString?(item: ItemType): string;
}

export interface InternationalProps<
MID = string,
ARGS = Record<string, unknown>
> {
translateWithId?(messageId: MID, args?: ARGS): string;
}

export interface MenuOffsetData {
left?: number | undefined;
top?: number | undefined;
}

export interface RenderIconProps<P = any> {
renderIcon?: React.ComponentType<P> | undefined;
}

export interface RequiresChildrenProps<T = React.ReactNode> {
children: NonNullable<T>;
}

export interface RequiresIdProps<T = ReactAttr['id']> {
id: NonNullable<T>;
}

export interface SizingProps {
small?: boolean | undefined;
}

export interface SideNavSharedProps {
isSideNavExpanded?: boolean | undefined;
}

export interface SideNavSizingProps {
large?: boolean | undefined;
}

//
// aliases for some React types that it doesn't export directly. They are needed to make sure we match the signatures
// as close as possible.
//
// reference patterns:
// function component with no generics: export declare const Comp: React.FC<PropsInterface>;
// function component with generics: export declare function Comp<T extends SomeType>(props: FCProps<PropsInterface<T>>): FCReturn;
// forwardRef component with no generics: export declare const Comp: ForwardRefReturn<HTMLElement, PropsInterface>;
// forwardRef component with generics: export declare function Comp<T extends SomeType>(props: ForwardRefProps<HTMLElement, PropsInterface<T>>): FCReturn;
//
export type FCProps<P = unknown> = Parameters<React.FC<P>>[0];
export type FCReturn = ReturnType<React.FC>;
export type ForwardRefProps<T, P = unknown> = React.PropsWithoutRef<
React.PropsWithChildren<P>
> &
React.RefAttributes<T>;
export type ForwardRefReturn<T, P = unknown> = React.ForwardRefExoticComponent<
ForwardRefProps<T, P>
>;

export type JSXIntrinsicElementProps<
K extends keyof JSX.IntrinsicElements,
REF extends boolean = false
> = REF extends true
? JSX.IntrinsicElements[K]
: Omit<JSX.IntrinsicElements[K], 'ref'>;

// for "as" props
export type ReactComponentConstructor<P> =
| ((props: P) => FCReturn)
| (new (props: P) => React.Component<unknown, any>);
28 changes: 28 additions & 0 deletions packages/react/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"compilerOptions": {
"composite": true,
"target": "es5",
"lib": ["dom", "dom.iterable", "esnext"],
"allowJs": true,
"skipLibCheck": true,
"esModuleInterop": true,
"allowSyntheticDefaultImports": true,
"strict": true,
"forceConsistentCasingInFileNames": true,
"module": "commonjs",
"moduleResolution": "node",
"isolatedModules": true,
"resolveJsonModule": true,
"noEmit": true,
"jsx": "react-jsx",
"sourceMap": true,
"declaration": true,
// use linting instead
"noUnusedLocals": false,
"noUnusedParameters": false,
"incremental": true,
"noFallthroughCasesInSwitch": true
},
"include": ["src/**/*"],
"exclude": ["node_modules", "build"]
}
Loading