Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix: names of mapped props for shorthand factories #591

Merged
kuzhelov merged 8 commits into from
Dec 13, 2018
Merged

fix: names of mapped props for shorthand factories #591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c21d3b5
introduce fix and type checks
kuzhelov-ms Dec 10, 2018
3be332a
remove invalid test for RadioGroup
kuzhelov-ms Dec 11, 2018
349353b
adjust typings
kuzhelov-ms Dec 11, 2018
ca2d2f0
Merge branch 'master' into fix/shorthand-factory-mapped-prop-names
kuzhelov Dec 11, 2018
9ed5929
Merge branch 'master' into fix/shorthand-factory-mapped-prop-names
kuzhelov Dec 11, 2018
595b576
address problems of RadioGroup tests
kuzhelov-ms Dec 11, 2018
ee50266
Merge branch 'master' into fix/shorthand-factory-mapped-prop-names
kuzhelov Dec 13, 2018
f4a4daf
update changelog
kuzhelov-ms Dec 13, 2018
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- Fix `Popup` not closing on outside click @kuzhelov ([#598](https://github.com/stardust-ui/react/pull/598))
- Fix multiple React's warnings about keys in docs @layershifter ([#602](https://github.com/stardust-ui/react/pull/602))
- Fix incorrect handling of `isFromKeyboard` in `Menu` @layershifter ([#596](https://github.com/stardust-ui/react/pull/596))
- Fix property names used in shorthand factories @kuzhelov ([#591](https://github.com/stardust-ui/react/pull/591))

### Features
- `Ref` components uses `forwardRef` API by default @layershifter ([#491](https://github.com/stardust-ui/react/pull/491))
Expand Down
3 changes: 3 additions & 0 deletions src/components/Image/Image.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ export interface ImageProps extends UIComponentProps {

/** An image can take up the width of its container. */
fluid?: boolean

/** Image source URL. */
src?: string
Copy link
Contributor

@mnajdova mnajdova Dec 12, 2018

Choose a reason for hiding this comment

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

👍

}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/components/ItemLayout/ItemLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ class ItemLayout extends UIComponent<Extendable<ItemLayoutProps>, any> {
}
}

ItemLayout.create = createShorthandFactory(ItemLayout, 'main')
ItemLayout.create = createShorthandFactory(ItemLayout, 'content')

export default ItemLayout

Expand Down
2 changes: 1 addition & 1 deletion src/components/List/ListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,6 @@ class ListItem extends UIComponent<Extendable<ListItemProps>, ListItemState> {
}
}

ListItem.create = createShorthandFactory(ListItem, 'main')
ListItem.create = createShorthandFactory(ListItem, 'content')

export default ListItem
9 changes: 6 additions & 3 deletions src/lib/factories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as React from 'react'
import {
ShorthandValue,
Props,
PropsOf,
ShorthandRenderCallback,
ShorthandRenderFunction,
ShorthandRenderer,
Expand Down Expand Up @@ -74,18 +75,20 @@ export function createShorthand(
// ============================================================
// Factory Creators
// ============================================================

/**
* @param {React.ReactType} Component A ReactClass or string
* @param {string} mappedProp A function that maps a primitive value to the Component props
* @returns {function} A shorthand factory function waiting for `val` and `defaultProps`.
*/
export function createShorthandFactory(Component: React.ReactType, mappedProp?: string) {
export function createShorthandFactory<T extends React.ReactType>(
Component: T,
mappedProp?: keyof PropsOf<T>,
) {
if (typeof Component !== 'function' && typeof Component !== 'string') {
throw new Error('createShorthandFactory() Component must be a string or function.')
}

return (val, options) => createShorthand(Component, mappedProp, val, options)
return (val, options) => createShorthand(Component, mappedProp as string, val, options)
}

// ============================================================
Expand Down
27 changes: 19 additions & 8 deletions test/specs/commonTests/implementsCollectionShorthandProp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,42 @@
import * as React from 'react'
import { mountWithProvider as mount } from 'test/utils'
import * as _ from 'lodash'
import { PropsOf } from 'types/utils'

export type CollectionShorthandTestOptions = {
mapsValueToProp?: string
skipArrayOfStrings?: boolean
export type CollectionShorthandTestOptions<TProps = any> = {
mapsValueToProp: keyof (TProps & React.HTMLProps<HTMLElement>) | false
Copy link
Contributor

@mnajdova mnajdova Dec 12, 2018

Choose a reason for hiding this comment

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

👍 for adding the false value as a option!

}

export const DefaultCollectionShorthandTestOptions: CollectionShorthandTestOptions = {
mapsValueToProp: 'content',
skipArrayOfStrings: false,
}

export default Component => {
export type CollectionShorthandPropTestsRunner<TComponent> = <
TShorthandComponent extends React.ComponentType
>(
shorthandProp: keyof PropsOf<TComponent>,
ShorthandComponent: TShorthandComponent,
options?: CollectionShorthandTestOptions<PropsOf<TShorthandComponent>>,
) => any

export type CollectionShorthandPropTestsFactory = <TComponent extends React.ComponentType>(
Component: TComponent,
) => CollectionShorthandPropTestsRunner<TComponent>

export default ((Component: React.ComponentType) => {
return function implementsCollectionShorthandProp(
shorthandPropertyName: string,
ShorthandComponent: React.ComponentType,
options: CollectionShorthandTestOptions = DefaultCollectionShorthandTestOptions,
) {
const { mapsValueToProp } = options
const mapsValueToProp = options.mapsValueToProp as string

describe(`shorthand property for '${ShorthandComponent.displayName}'`, () => {
test(`is defined`, () => {
expect(Component.propTypes[shorthandPropertyName]).toBeTruthy()
})

if (!options.skipArrayOfStrings) {
if (options.mapsValueToProp) {
test(`array of string values is spread as ${
ShorthandComponent.displayName
}s' ${mapsValueToProp}`, () => {
Expand Down Expand Up @@ -73,4 +84,4 @@ export default Component => {
})
})
}
}
}) as CollectionShorthandPropTestsFactory
32 changes: 23 additions & 9 deletions test/specs/commonTests/implementsShorthandProp.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,35 @@
import * as React from 'react'
import { ReactWrapper } from 'enzyme'
import { mountWithProvider } from 'test/utils'
import { Props } from '../../../types/utils'
import { Props, PropsOf } from '../../../types/utils'

export type ShorthandTestOptions = {
mapsValueToProp?: string
export type ShorthandTestOptions<TProps = any> = {
mapsValueToProp: keyof (TProps & React.HTMLProps<HTMLElement>) | false
}

export const DefaultShorthandTestOptions: ShorthandTestOptions = {
mapsValueToProp: 'content',
}

export default Component => {
export type ShorthandPropTestsRunner<TComponent> = <
TShorthandComponent extends React.ComponentType
>(
shorthandProp: keyof PropsOf<TComponent>,
ShorthandComponent: TShorthandComponent,
options?: ShorthandTestOptions<PropsOf<TShorthandComponent>>,
) => any

export type ShorthandPropTestsFactory = <TComponent extends React.ComponentType>(
Component: TComponent,
) => ShorthandPropTestsRunner<TComponent>

export default ((Component: React.ComponentType) => {
return function implementsShorthandProp(
shorthandProp: string,
ShorthandComponent: React.ComponentType,
options: ShorthandTestOptions = DefaultShorthandTestOptions,
) {
const { mapsValueToProp } = options
const mapsValueToProp = options.mapsValueToProp as string
const { displayName } = ShorthandComponent

const checkPropsMatch = (props: Props, matchedProps: Props) =>
Expand Down Expand Up @@ -46,13 +58,15 @@ export default Component => {
expect(Component.propTypes[shorthandProp]).toBeTruthy()
})

test(`string value is handled as ${displayName}'s ${mapsValueToProp}`, () => {
expectShorthandPropsAreHandled('shorthand prop value')
})
if (options.mapsValueToProp) {
test(`string value is handled as ${displayName}'s ${mapsValueToProp}`, () => {
expectShorthandPropsAreHandled('shorthand prop value')
})
}

test(`object value is spread as ${displayName}'s props`, () => {
expectShorthandPropsAreHandled({ foo: 'foo value', bar: 'bar value' })
})
})
}
}
}) as ShorthandPropTestsFactory
2 changes: 1 addition & 1 deletion test/specs/components/List/List-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ const listImplementsCollectionShorthandProp = implementsCollectionShorthandProp(
describe('List', () => {
isConformant(List)
handlesAccessibility(List, { defaultRootRole: 'list' })
listImplementsCollectionShorthandProp('items', ListItem, { mapsValueToProp: 'main' })
listImplementsCollectionShorthandProp('items', ListItem, { mapsValueToProp: 'content' })
})
3 changes: 1 addition & 2 deletions test/specs/components/RadioGroup/RadioGroup-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ describe('RadioGroup', () => {

describe('implementsCollectionShorthandProp', () => {
radioGroupImplementsCollectionShorthandProp('items', RadioGroupItem, {
mapsValueToProp: 'content',
skipArrayOfStrings: true,
mapsValueToProp: false,
})
})

Expand Down
2 changes: 1 addition & 1 deletion test/specs/lib/factories-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describe('factories', () => {
})

test('does not throw if passed a function Component', () => {
const goodUsage = () => createShorthandFactory(() => <div />, '')
const goodUsage = () => createShorthandFactory(() => <div />, 'children')

expect(goodUsage).not.toThrowError()
})
Expand Down
11 changes: 11 additions & 0 deletions types/utils.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,17 @@ export type Props = ObjectOf<any>
export type ReactChildren = React.ReactNodeArray | React.ReactNode
export type ComponentEventHandler<TProps> = (event: React.SyntheticEvent, data: TProps) => void

type ChildrenProps = { children: any }
export type PropsOf<T> = T extends React.ComponentClass<Extendable<infer TProps>>
? (ChildrenProps & TProps)
: T extends React.ComponentClass<infer TProps>
? (ChildrenProps & TProps)
: T extends React.StatelessComponent<Extendable<infer TProps>>
? (ChildrenProps & TProps)
: T extends React.StatelessComponent<infer TProps>
? (ChildrenProps & TProps)
: any
Copy link
Member

@layershifter layershifter Dec 11, 2018

Choose a reason for hiding this comment

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

This type looks like a pure evil 😈 We had a similar issue before and I solved it with a generic type:

export function createShorthandFactory<P>(
  Component: React.ReactType<P>,
  mappedProp?: keyof P & string, // `string` may be redundant there
) {

Yes, we should update all files, but this type check is much simpler and obvious.

Copy link
Contributor Author

@kuzhelov kuzhelov Dec 11, 2018

Choose a reason for hiding this comment

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

@layershifter

for the suggested approach

  • string type should be definitely eliminated from the intersection type in either case, because, otherwise, it will just match all the cases we were struggling before
  • for the rest of the suggested part - unfortunately, type inference for the expression you are suggesting won't take place - because compiler will be pretty happy to use any as type for P and this will satisfy all the conditions of the typing expressions used in the example. To really utilize type inference we have no other options than use infer in type declarations.

I don't think that this type looks unreadable - actually, am pretty sure that any infer expression suggested in either TS guide or Deep Dive book would look similar.

At the same time, I've intentionally made these changes to be unobtrusive - you might notice that I've affected public interfaces of the methods only, without touching the actual types that were used before inside of the methods. Have done this intentionally, to be sure that this type guard functionality could be easily modified/fixed/removed if necessary.

Copy link
Contributor Author

@kuzhelov kuzhelov Dec 11, 2018

Choose a reason for hiding this comment

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

yes, this is, actually, from the TS docs: https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#generic-rest-parameters

type Unpacked<T> =
    T extends (infer U)[] ? U :
    T extends (...args: any[]) => infer U ? U :
    T extends Promise<infer U> ? U :
    T;

don't get me wrong - I would be glad to use simpler expression to achieve the same goal (and opened to any suggestions), just now see it as the only (reliable) solution available to us

Copy link
Contributor Author

@kuzhelov kuzhelov Dec 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

oh, and now, probably, see another reason for your point (apart from the general one) - thing is that I've used another formatting initially:

T extends React.ComponentClass<Extendable<infer TProps>> ? (ChildrenProps & TProps) 
  : T extends React.ComponentClass<infer TProps> ? (ChildrenProps & TProps) 
  : T extends React.StatelessComponent<Extendable<infer TProps>> ? (ChildrenProps & TProps) 
  : T extends React.StatelessComponent<infer TProps> ? (ChildrenProps & TProps)
  : any

This was easier to read - but now, with the prettier changes it becomes absolutely cryptic. Will try to address this readability issue

Copy link
Contributor

@mnajdova mnajdova Dec 11, 2018

Choose a reason for hiding this comment

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

I've tested this myself locally too. @layershifter your proposal works, but as you mention we will need to update all createShorthandFactory usages, if we want to leverage this. If we forget to do this, we may still have the previous errors. @kuzhelov 's suggested typings don't require this from the user, and offer out of the box checking for this. I see this as a better alternative and big improvement step. I vote for the typings implemented in the PR.

Copy link
Member

@layershifter layershifter Dec 11, 2018

Choose a reason for hiding this comment

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

@mnajdova I agree and fully support improvals there, but how much time do you need to understand what is happening in there? To be honest, can you say that this type is a simple and obvious?

Copy link
Contributor

@mnajdova mnajdova Dec 12, 2018

Choose a reason for hiding this comment

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

I am not saying it is simple and obvious, but I don't think that just because it is not simple, we should not add it. Improving our skills and knowledge will mean we will start adding more complicated typings, and I am totally supporting that. @kuzhelov maybe just short description above the type will help others to better understand it.

Copy link
Member

@miroslavstastny miroslavstastny Dec 13, 2018

Choose a reason for hiding this comment

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

This is insane! 👍

Copy link
Contributor Author

@kuzhelov kuzhelov Dec 13, 2018

Choose a reason for hiding this comment

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

have tried alternative options, but all of them require type inference and, thus, the same amount of infer and related ternary operators. Lets leave this approach for now, as we have functionality that is necessary for us and it will be encapsulated under meaningful name. We can return to that and refactor later if necessary


// ========================================================
// Shorthand Factories
// ========================================================
Expand Down