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

Addon-knobs: improve types via generics and readonlyarray #7411

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions addons/knobs/src/KnobManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { getQueryParams } from '@storybook/client-api';
import { Channel } from '@storybook/channels';

import KnobStore, { KnobStoreKnob } from './KnobStore';
import { Knob, KnobType } from './type-defs';
import { Knob, KnobType, Mutable } from './type-defs';
import { SET } from './shared';

import { deserializers } from './converters';
Expand Down Expand Up @@ -72,7 +72,7 @@ export default class KnobManager {
return this.options.escapeHTML ? escapeStrings(value) : value;
}

knob<T extends KnobType = any>(name: string, options: Knob<T>): Knob<T>['value'] {
knob<T extends KnobType = any>(name: string, options: Knob<T>): Mutable<Knob<T>['value']> {
this._mayCallChannel();

const knobName = options.groupId ? `${name}_${options.groupId}` : name;
Expand Down
204 changes: 204 additions & 0 deletions addons/knobs/src/__types__/knob-test-cases.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
import {
number,
color,
files,
object,
boolean,
text,
select,
date,
array,
button,
knob,
radios,
optionsKnob as options,
} from '../index';

// Note: this is a helper to batch test return types and avoid "declared but never read" errors
function expectKnobOfType<T>(..._: T[]) {}

const groupId = 'GROUP-ID1';

/** Text knob */

expectKnobOfType<string>(
text('text simple', 'Batman'),
text('text with group', 'default', groupId)
);

/** Date knob */

expectKnobOfType<number>(
date('date simple', new Date('January 20 1887')),
date('date with group', new Date(), groupId)
);

/** Boolean knob */

expectKnobOfType<boolean>(
boolean('boolean simple', false),
boolean('boolean with group', true, groupId)
);

/** Color knob */

expectKnobOfType<string>(
color('color simple', 'black'),
color('color with group', '#ffffff', groupId)
);

/** Number knob */

expectKnobOfType<number>(
number('number basic', 42),
number('number with options', 72, { range: true, min: 60, max: 90, step: 1 }),
number('number with group', 1, {}, groupId)
);

/** Radios knob */

expectKnobOfType(
radios<string>(
'radio with string values',
{
1100: '1100',
2200: '2200',
3300: '3300',
},
'2200'
),
radios<number>('radio with number values', { 3: 3, 7: 7, 23: 23 }, 3),
radios(
'radio with mixed value',
{
1100: '1100',
2200: 2200,
3300: '3300',
},
null,
groupId
)
);

/** Select knob */

enum SomeEnum {
Type1 = 1,
Type2,
}
enum ButtonVariant {
primary = 'primary',
secondary = 'secondary',
}
const stringLiteralArray: ('Apple' | 'Banana' | 'Grapes')[] = ['Apple', 'Banana', 'Grapes'];

expectKnobOfType<string>(
select(
'select with string options',
{
None: 'none',
Underline: 'underline',
'Line-through': 'line-through',
},
'none'
),
select('select with string array', ['yes', 'no'], 'yes'),
select('select with string literal array', stringLiteralArray, stringLiteralArray[0]),
select('select with readonly array', ['red', 'blue'] as const, 'red'),
select<ButtonVariant>('select with string enum options', ButtonVariant, ButtonVariant.primary),
select('select with null option', { a: 'Option', b: null }, null, groupId)
);

expectKnobOfType<number>(
select('select with number options', { 'type a': 1, 'type b': 2 }, 1),
select<SomeEnum>(
'select with numeric enum options',
{ 'type a': SomeEnum.Type1, 'type b': SomeEnum.Type2 },
SomeEnum.Type2
),
select('select with number array', [1, 2, 3, 4], 1),
select('select with readonly number array', [1, 2] as const, 1)
);

/** Object knob */

expectKnobOfType(
object('object simple', {
fontFamily: 'Arial',
padding: 20,
}),
object('object with group', {}, groupId)
);

/** Options knob */

type Tool = 'hammer' | 'saw' | 'drill';
const visibleToolOptions: Record<string, Tool> = {
hammer: 'hammer',
saw: 'saw',
drill: 'drill',
};

expectKnobOfType(
options<Tool>('options with single selection', visibleToolOptions, 'hammer', {
display: 'check',
}),
options<Tool>('options with multi selection', visibleToolOptions, ['hammer', 'saw'], {
display: 'inline-check',
}),
options<Tool>('options with readonly multi selection', visibleToolOptions, ['hammer'] as const, {
display: 'radio',
}),
options('options with group', {}, '', { display: 'check' })
);

/** Array knob */

const arrayReadonly = array('array as readonly', ['hi', 'there'] as const);

expectKnobOfType<string[]>(
array('array simple', ['red', 'green', 'blue']),
arrayReadonly,
array('array with group', [], ',', groupId)
);

// Should return a mutable array despite the readonly input
arrayReadonly.push('Make sure that the output is still mutable although the input need not be!');

/** Button knob */

expectKnobOfType(
button('button simple', () => {}),
button('button with group', () => undefined, groupId)
);

/** Files knob */

expectKnobOfType<string[]>(
files('files simple', 'image/*', []),
files('files with group', 'image/*', ['img.jpg'], groupId)
);

/** Generic knob */

expectKnobOfType<string>(
knob('generic knob as text', { type: 'text', value: 'a' }),
knob('generic knob as color', { type: 'color', value: 'black' }),
knob<'select', string>('generic knob as string select', {
type: 'select',
value: 'yes',
options: ['yes', 'no'],
selectV2: true,
})
);

expectKnobOfType<number>(
knob('generic knob as number', { type: 'number', value: 42 }),
knob('generic knob as select', { type: 'radios', value: 3, options: { 3: 3, 7: 7, 23: 23 } }),
knob('generic knob as number select', {
type: 'select',
value: 1,
options: [1, 2],
selectV2: true,
})
);
12 changes: 5 additions & 7 deletions addons/knobs/src/components/types/Array.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@ import PropTypes from 'prop-types';
import React, { ChangeEvent, Component, WeakValidationMap } from 'react';

import { Form } from '@storybook/components';
import { KnobControlConfig, KnobControlProps } from './types';

type ArrayTypeKnobValue = string[];
export type ArrayTypeKnobValue = string[] | readonly string[];
Copy link

@nickofthyme nickofthyme Jul 29, 2019

Choose a reason for hiding this comment

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

@emilio-martinez Is this breaking type change intentional? Using numbers will cause a typescript error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To quote the README for this add-on:

array
Allows you to get an array of strings from the user.

Unfortunately, the package from DefinitelyTyped is inaccurate—that field uses a text-based input of comma delimited values, so even if you were to pass a number array initially, it would subsequently return a string array on change, i.e., here's no number parsing logic for that control.

That said, I'd love to hear more about your use case and help in any way I can. Would you mind sharing?

Copy link

@nickofthyme nickofthyme Jul 30, 2019

Choose a reason for hiding this comment

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

Hey @emilio-martinez Thanks for the reply. Yeah it seems like you guys are just having to bite the bullet on these type changes going from javascript to typescript, which I totally feel your pain.

This is my use case 👇. It's fixable by just parsing the values and there is probably a better way to go about it. Just got this error when upgrading from v5.1.9 to v5.2.0-beta.19.
https://github.com/elastic/elastic-charts/blob/8ebe600df1ae476e25032ab1c7707de398143e43/stories/annotations.tsx#L41

I'm curious if you guys are gradually upgrading you codebase to ts or doing it in a single release (I'm too lazy to check for myself)? If it is in a single release why wouldn’t you just release ts in v6 rather than producing all these stricter type errors on a minor release?

Thanks 👍

Copy link
Member

Choose a reason for hiding this comment

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

TS migration is an epic started at the end of 2018, we recently reached an important step as 50% of Storybook codebase in TS 🎉.
Migration progress is followed in #5030 😉. Avoiding breaking changes is one of our goals but we somethings found weird/wrong types/docs even in SB own codebase.

Choose a reason for hiding this comment

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

@gaetanmaisse Sweet congrats on the big 5-0!! 🎉 All the best keep up the great work.

Copy link
Contributor

@leoyli leoyli Jul 31, 2019

Choose a reason for hiding this comment

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

Actually, there are many work around and we don't buy in strict type and noImplicitAny at day one (I think is for easier migration). For that reason, there are sheer amount of any flood in our typing. That somehow explains why we inevitably will have breaking changes in type system, we need to improve it gradually due to the fact that the codebase is pretty huge. But I think this brings more organization and community collaboration to make it awesome in long term.


export interface ArrayTypeKnob {
name: string;
value: ArrayTypeKnobValue;
export interface ArrayTypeKnob extends KnobControlConfig<ArrayTypeKnobValue> {
separator: string;
}

interface ArrayTypeProps {
interface ArrayTypeProps extends KnobControlProps<ArrayTypeKnobValue> {
knob: ArrayTypeKnob;
onChange: (value: ArrayTypeKnobValue) => ArrayTypeKnobValue;
}

function formatArray(value: string, separator: string) {
Expand Down Expand Up @@ -41,7 +39,7 @@ export default class ArrayType extends Component<ArrayTypeProps> {

static serialize = (value: ArrayTypeKnobValue) => value;

static deserialize = (value: ArrayTypeKnobValue) => {
static deserialize = (value: string[]) => {
if (Array.isArray(value)) return value;

return Object.keys(value)
Expand Down
10 changes: 3 additions & 7 deletions addons/knobs/src/components/types/Boolean.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,14 @@ import PropTypes from 'prop-types';
import React, { FunctionComponent } from 'react';

import { styled } from '@storybook/theming';
import { KnobControlConfig, KnobControlProps } from './types';

type BooleanTypeKnobValue = boolean;

export interface BooleanTypeKnob {
name: string;
value: BooleanTypeKnobValue;
separator: string;
}
export type BooleanTypeKnob = KnobControlConfig<BooleanTypeKnobValue>;

export interface BooleanTypeProps {
export interface BooleanTypeProps extends KnobControlProps<BooleanTypeKnobValue> {
knob: BooleanTypeKnob;
onChange: (value: BooleanTypeKnobValue) => BooleanTypeKnobValue;
}

const Input = styled.input({
Expand Down
12 changes: 5 additions & 7 deletions addons/knobs/src/components/types/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,17 @@ import PropTypes from 'prop-types';
import React, { FunctionComponent, Validator } from 'react';

import { Form } from '@storybook/components';
import { KnobControlConfig, KnobControlProps } from './types';

export interface ButtonTypeKnob {
name: string;
value: unknown;
}

export type ButtonTypeOnClickProp = (knob: ButtonTypeKnob) => any;
export type ButtonTypeKnob = KnobControlConfig<never>;

export interface ButtonTypeProps {
export interface ButtonTypeProps extends KnobControlProps<never> {
knob: ButtonTypeKnob;
onClick: ButtonTypeOnClickProp;
}

export type ButtonTypeOnClickProp = (knob: ButtonTypeKnob) => any;

const serialize = (): undefined => undefined;
const deserialize = (): undefined => undefined;

Expand Down
24 changes: 10 additions & 14 deletions addons/knobs/src/components/types/Checkboxes.tsx
Original file line number Diff line number Diff line change
@@ -1,32 +1,28 @@
import React, { Component, ChangeEvent, WeakValidationMap } from 'react';
import PropTypes from 'prop-types';
import { styled } from '@storybook/theming';
import { KnobControlConfig, KnobControlProps } from './types';

type CheckboxesTypeKnobValue = string[];

interface CheckboxesWrapperProps {
isInline: boolean;
}

export interface CheckboxesTypeKnob {
name: string;
value: CheckboxesTypeKnobValue;
defaultValue: CheckboxesTypeKnobValue;
options: {
[key: string]: string;
};
export interface CheckboxesTypeKnob extends KnobControlConfig<CheckboxesTypeKnobValue> {
options: Record<string, string>;
}

interface CheckboxesTypeProps {
interface CheckboxesTypeProps
extends KnobControlProps<CheckboxesTypeKnobValue>,
CheckboxesWrapperProps {
knob: CheckboxesTypeKnob;
isInline: boolean;
onChange: (value: CheckboxesTypeKnobValue) => CheckboxesTypeKnobValue;
}

interface CheckboxesTypeState {
values: CheckboxesTypeKnobValue;
}

interface CheckboxesWrapperProps {
isInline: boolean;
}

const CheckboxesWrapper = styled.div(({ isInline }: CheckboxesWrapperProps) =>
isInline
? {
Expand Down
13 changes: 3 additions & 10 deletions addons/knobs/src/components/types/Color.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,11 @@ import { SketchPicker, ColorResult } from 'react-color';

import { styled } from '@storybook/theming';
import { Form } from '@storybook/components';
import { KnobControlConfig, KnobControlProps } from './types';

type ColorTypeKnobValue = string;

export interface ColorTypeKnob {
name: string;
value: ColorTypeKnobValue;
}

interface ColorTypeProps {
knob: ColorTypeKnob;
onChange: (value: ColorTypeKnobValue) => ColorTypeKnobValue;
}
export type ColorTypeKnob = KnobControlConfig<ColorTypeKnobValue>;
type ColorTypeProps = KnobControlProps<ColorTypeKnobValue>;

interface ColorTypeState {
displayColorPicker: boolean;
Expand Down
13 changes: 3 additions & 10 deletions addons/knobs/src/components/types/Date.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,11 @@ import React, { Component, ChangeEvent, WeakValidationMap } from 'react';
import PropTypes from 'prop-types';
import { styled } from '@storybook/theming';
import { Form } from '@storybook/components';
import { KnobControlConfig, KnobControlProps } from './types';

type DateTypeKnobValue = number;

export interface DateTypeKnob {
name: string;
value: DateTypeKnobValue;
}

interface DateTypeProps {
knob: DateTypeKnob;
onChange: (value: DateTypeKnobValue) => DateTypeKnobValue;
}
export type DateTypeKnob = KnobControlConfig<DateTypeKnobValue>;
type DateTypeProps = KnobControlProps<DateTypeKnobValue>;

interface DateTypeState {
valid: boolean | undefined;
Expand Down
Loading