From c60dfd532b00600a45077f40e7d6e61ddd39e76a Mon Sep 17 00:00:00 2001 From: Levi Thomason Date: Fri, 20 Jul 2018 16:28:38 -0700 Subject: [PATCH] feat: add generateKey option to factories (#3) * feat: add generateKey option to factories * chore(codecov): ignore test directory * chore(circle): do no install puppeteer --- .circleci/config.yml | 12 --- codecov.yml | 1 + specifications/shorthand-api.md | 32 +++++- src/components/Accordion/Accordion.tsx | 4 +- src/components/Header/Header.tsx | 2 +- src/lib/factories.tsx | 97 +++++++++++-------- src/lib/renderComponent.tsx | 2 +- .../commonTests/implementsShorthandProp.tsx | 14 ++- test/specs/lib/factories-test.tsx | 64 ++++++------ 9 files changed, 135 insertions(+), 93 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 44f635280a..3b2032d62a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -10,18 +10,6 @@ jobs: environment: TZ: "/usr/share/zoneinfo/America/Los_Angeles" steps: - # Chrome HeadlessBrowser is missing deps on Debian, see: - # https://github.com/GoogleChrome/puppeteer/issues/290 - - run: - name: Update Puppeteer Dependencies - command: | - sudo apt-get update - sudo apt-get install --yes --quiet gconf-service libasound2 libatk1.0-0 libc6 \ - libcairo2 libcups2 libdbus-1-3 libexpat1 libfontconfig1 libgcc1 libgconf-2-4 \ - libgdk-pixbuf2.0-0 libglib2.0-0 libgtk-3-0 libnspr4 libpango-1.0-0 libpangocairo-1.0-0 \ - libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 libxcursor1 libxdamage1 libxext6 \ - libxfixes3 libxi6 libxrandr2 libxrender1 libxss1 libxtst6 ca-certificates fonts-liberation \ - libappindicator1 libnss3 lsb-release xdg-utils wget - run: name: Update yarn command: | diff --git a/codecov.yml b/codecov.yml index 5ef6318197..a00faf897e 100644 --- a/codecov.yml +++ b/codecov.yml @@ -2,3 +2,4 @@ coverage: ignore: - docs/* - src/lib/* + - test/* diff --git a/specifications/shorthand-api.md b/specifications/shorthand-api.md index 205b80d4a8..231b1fcf6c 100644 --- a/specifications/shorthand-api.md +++ b/specifications/shorthand-api.md @@ -5,17 +5,39 @@ - [Why?](#why) + - [DOM Structure](#dom-structure) + - [A Moving Target](#a-moving-target) + - [Ownership](#ownership) + - [Intuition & Effort](#intuition--effort) - [Proposals](#proposals) - - [[Goal]](#goal) + - [Goal](#goal) ## Why? -## Proposals +### DOM Structure + +In traditional UI libraries, developers are required to memorize or copy and paste specific various brittle HTML structures to use components. The developer should be focused on a higher level of describing features. + +#### A Moving Target + +These structures are often required for the component to work correctly. This makes them brittle and a common source of bugs. This is only exacerbated by the fact that different variations of the same component often require slightly different structures. + +The component should own and isolate the brittle nature of required markup. + +#### Ownership -### [Goal] +Developers require components to have certain styles and behaviors. Components may require a specific DOM structure to achieve those styles and behaviors. Therefore, component DOM structure is the concern and responsibility of the component, not the user. + +### Intuition & Effort + +When describing a component to another human we use natural language, we don't speak HTML. When defining a component via an API we should strive to use the same natural language. This frees the developer's mind to spend its effort creating actual features opposed to creating implementations of features. + +Providing a high level API of natural language allows developers to use intuition and minimal effort to create features. + +## Proposals -[Description] +See the [docs][1]. -[Implementation] +[1]: https://stardust-ui.github.io/react diff --git a/src/components/Accordion/Accordion.tsx b/src/components/Accordion/Accordion.tsx index 9a10c3315e..73e798b276 100644 --- a/src/components/Accordion/Accordion.tsx +++ b/src/components/Accordion/Accordion.tsx @@ -124,7 +124,7 @@ class Accordion extends AutoControlledComponent { children.push( AccordionTitle.create(title, { - autoGenerateKey: true, + generateKey: true, defaultProps: { active, index }, overrideProps: this.handleTitleOverrides, }), @@ -133,7 +133,7 @@ class Accordion extends AutoControlledComponent { AccordionContent.create( { content }, { - autoGenerateKey: true, + generateKey: true, defaultProps: { active }, }, ), diff --git a/src/components/Header/Header.tsx b/src/components/Header/Header.tsx index 05e977e989..7b5dc578c1 100644 --- a/src/components/Header/Header.tsx +++ b/src/components/Header/Header.tsx @@ -53,7 +53,7 @@ class Header extends UIComponent { ) } - const subheaderElement = HeaderSubheader.create(subheader, { autoGenerateKey: false }) + const subheaderElement = HeaderSubheader.create(subheader, { generateKey: false }) return ( diff --git a/src/lib/factories.tsx b/src/lib/factories.tsx index 7a20502ce4..24d408fbcc 100644 --- a/src/lib/factories.tsx +++ b/src/lib/factories.tsx @@ -2,48 +2,64 @@ import _ from 'lodash' import cx from 'classnames' import React, { cloneElement, isValidElement } from 'react' +interface IProps { + [key: string]: any +} + +type ShorthandFunction = ( + Component: React.ReactType, + props: IProps, + children: any, +) => React.ReactElement +type ShorthandValue = string & number & IProps & React.ReactElement & ShorthandFunction +type MapValueToProps = (value: ShorthandValue) => IProps + +interface ICreateShorthandOptions { + /** Default props object */ + defaultProps?: IProps + + /** Override props object or function (called with regular props) */ + overrideProps?: IProps | ((props: IProps) => IProps) + + /** Whether or not automatic key generation is allowed */ + generateKey?: boolean +} +const CREATE_SHORTHAND_DEFAULT_OPTIONS: ICreateShorthandOptions = { + defaultProps: {}, + overrideProps: {}, + generateKey: true, +} + // ============================================================ // Factories // ============================================================ -/** - * A more robust React.createElement. It can create elements from primitive values. - * - * @param {function|string} Component A ReactClass or string - * @param {function} mapValueToProps A function that maps a primitive value to the Component props - * @param {string|object|function} val The value to create a ReactElement from - * @param {Object} [options={}] - * @param {object} [options.defaultProps={}] Default props object - * @param {object|function} [options.overrideProps={}] Override props object or function (called with regular props) - * @returns {object|null} - */ +/** A more robust React.createElement. It can create elements from primitive values. */ export function createShorthand( - Component: any, - mapValueToProps: any, - val?: any, - options: any = {}, -) { + Component: React.ReactType, + mapValueToProps: MapValueToProps, + value?: ShorthandValue, + options: ICreateShorthandOptions = CREATE_SHORTHAND_DEFAULT_OPTIONS, +): React.ReactElement | null { if (typeof Component !== 'function' && typeof Component !== 'string') { throw new Error('createShorthand() Component must be a string or function.') } // short circuit noop values - if (_.isNil(val) || _.isBoolean(val)) return null + if (_.isNil(value) || typeof value === 'boolean') return null - const valIsString = _.isString(val) - const valIsNumber = _.isNumber(val) - const valIsFunction = _.isFunction(val) - const valIsReactElement = isValidElement(val) - const valIsPropsObject = _.isPlainObject(val) - const valIsPrimitiveValue = valIsString || valIsNumber || _.isArray(val) + const valIsPrimitive = typeof value === 'string' || typeof value === 'number' + const valIsPropsObject = _.isPlainObject(value) + const valIsReactElement = isValidElement(value) + const valIsFunction = typeof value === 'function' // unhandled type return null - if (!valIsFunction && !valIsReactElement && !valIsPropsObject && !valIsPrimitiveValue) { + if (!valIsPrimitive && !valIsPropsObject && !valIsReactElement && !valIsFunction) { if (process.env.NODE_ENV !== 'production') { console.error( [ - 'Shorthand value must be a string|number|array|object|ReactElement|function.', - ' Use null|undefined|boolean for none', - ` Received ${typeof val}.`, + 'Shorthand value must be a string|number|object|ReactElement|function.', + ' Use null|undefined|boolean for none.', + ` Received: ${value}`, ].join(''), ) } @@ -57,15 +73,17 @@ export function createShorthand( // User's props const usersProps = - (valIsReactElement && val.props) || - (valIsPropsObject && val) || - (valIsPrimitiveValue && mapValueToProps(val)) + (valIsReactElement && value.props) || + (valIsPropsObject && value) || + (valIsPrimitive && mapValueToProps(value)) || + {} // Override props - let { overrideProps = {} } = options - overrideProps = _.isFunction(overrideProps) - ? overrideProps({ ...defaultProps, ...usersProps }) - : overrideProps + let { overrideProps } = options + overrideProps = + typeof overrideProps === 'function' + ? overrideProps({ ...defaultProps, ...usersProps }) + : overrideProps || {} // Merge props const props = { ...defaultProps, ...usersProps, ...overrideProps } @@ -88,11 +106,12 @@ export function createShorthand( // ---------------------------------------- // Get key // ---------------------------------------- + const { generateKey = true } = options // Use key or generate key - if (_.isNil(props.key) && (valIsString || valIsNumber)) { + if (generateKey && _.isNil(props.key) && valIsPrimitive) { // use string/number shorthand values as the key - props.key = val + props.key = value } // ---------------------------------------- @@ -100,13 +119,13 @@ export function createShorthand( // ---------------------------------------- // Clone ReactElements - if (valIsReactElement) return cloneElement(val, props) + if (valIsReactElement) return cloneElement(value, props) // Create ReactElements from built up props - if (valIsPrimitiveValue || valIsPropsObject) return + if (valIsPrimitive || valIsPropsObject) return // Call functions with args similar to createElement() - if (valIsFunction) return val(Component, props, props.children) + if (valIsFunction) return value(Component, props, props.children) } // ============================================================ diff --git a/src/lib/renderComponent.tsx b/src/lib/renderComponent.tsx index aa98caf579..3dbd717199 100644 --- a/src/lib/renderComponent.tsx +++ b/src/lib/renderComponent.tsx @@ -8,7 +8,7 @@ import getUnhandledProps from './getUnhandledProps' import callable from './callable' export interface IRenderResultConfig

{ - ElementType: React.ComponentType

+ ElementType: React.ReactType

rest: { [key: string]: any } classes: { [key: string]: string } } diff --git a/test/specs/commonTests/implementsShorthandProp.tsx b/test/specs/commonTests/implementsShorthandProp.tsx index fc7c0a4e28..4bef6a1546 100644 --- a/test/specs/commonTests/implementsShorthandProp.tsx +++ b/test/specs/commonTests/implementsShorthandProp.tsx @@ -25,6 +25,7 @@ const shorthandComponentName = ShorthandComponent => { * @param {string|function} options.ShorthandComponent The component that should be rendered from the shorthand value. * @param {boolean} [options.alwaysPresent] Whether or not the shorthand exists by default. * @param {boolean} [options.assertExactMatch] Selects an assertion method, `contain` will be used if true. + * @param {boolean} [options.generateKey=false] Whether or not automatic key generation is allowed for the shorthand component. * @param {function} options.mapValueToProps A function that maps a primitive value to the Component props. * @param {Object} [options.requiredProps={}] Props required to render the component. * @param {Object} [options.shorthandDefaultProps] Default props for the shorthand component. @@ -34,6 +35,7 @@ export default (Component, options: any = {}) => { const { alwaysPresent, assertExactMatch = true, + generateKey = false, mapValueToProps, propKey, ShorthandComponent, @@ -52,13 +54,21 @@ export default (Component, options: any = {}) => { const name = shorthandComponentName(ShorthandComponent) const assertValidShorthand = value => { - const shorthandElement = createShorthand(ShorthandComponent, mapValueToProps, value, { + const expectedShorthandElement = createShorthand(ShorthandComponent, mapValueToProps, value, { defaultProps: shorthandDefaultProps, overrideProps: shorthandOverrideProps, + generateKey, }) const element = createElement(Component, { ...requiredProps, [propKey]: value }) + const wrapper = shallow(element) - shallow(element).should[assertMethod](shorthandElement) + wrapper.should[assertMethod](expectedShorthandElement) + + // Enzyme's .key() method is not consistent with React for elements with + // no key (`undefined` vs `null`), so use the underlying element instead + // Will fail if more than one element of this type is found + const shorthandElement = wrapper.find(ShorthandComponent).getElement() + expect(shorthandElement.key).to.equal(expectedShorthandElement.key, "key doesn't match") } if (alwaysPresent || (Component.defaultProps && Component.defaultProps[propKey])) { diff --git a/test/specs/lib/factories-test.tsx b/test/specs/lib/factories-test.tsx index 0a7809fbef..728a6f9d5b 100644 --- a/test/specs/lib/factories-test.tsx +++ b/test/specs/lib/factories-test.tsx @@ -15,8 +15,14 @@ const getShorthand = ({ defaultProps, mapValueToProps = () => ({}), overrideProps, + generateKey, value, -}: any) => createShorthand(Component, mapValueToProps, value, { defaultProps, overrideProps }) +}: any) => + createShorthand(Component, mapValueToProps, value, { + defaultProps, + overrideProps, + generateKey, + }) // ---------------------------------------- // Common tests @@ -24,12 +30,14 @@ const getShorthand = ({ const itReturnsNull = value => { test('returns null', () => { + consoleUtil.disableOnce() expect(getShorthand({ value })).toBe(null) }) } const itReturnsNullGivenDefaultProps = value => { test('returns null given defaultProps object', () => { + consoleUtil.disableOnce() expect(getShorthand({ value, defaultProps: { 'data-foo': 'foo' } })).toBe(null) }) } @@ -120,6 +128,7 @@ describe('factories', () => { }) test('throw if passed Component that is not a string nor function', () => { + consoleUtil.disableOnce() const badComponents = [undefined, null, true, false, [], {}, 123] _.each(badComponents, badComponent => { @@ -148,6 +157,7 @@ describe('factories', () => { }) test('throw if passed Component that is not a string nor function', () => { + consoleUtil.disableOnce() const badComponents = [undefined, null, true, false, [], {}, 123] _.each(badComponents, badComponent => { @@ -209,6 +219,26 @@ describe('factories', () => { expect(getShorthand({ value: { key: '' } })).toHaveProperty('key', '') }) }) + + describe('when value is a string', () => { + test('is generated from the value', () => { + expect(getShorthand({ value: 'foo' })).toHaveProperty('key', 'foo') + }) + + test('is not generated if generateKey is false', () => { + expect(getShorthand({ value: 'foo', generateKey: false })).toHaveProperty('key', null) + }) + }) + + describe('when value is a number', () => { + test('is generated from the value', () => { + expect(getShorthand({ value: 123 })).toHaveProperty('key', '123') + }) + + test('is not generated if generateKey is false', () => { + expect(getShorthand({ value: 123, generateKey: false })).toHaveProperty('key', null) + }) + }) }) describe('overrideProps', () => { @@ -390,36 +420,8 @@ describe('factories', () => { }) describe('from an array', () => { - itReturnsAValidElement(['foo']) - itAppliesDefaultProps(['foo']) - itMergesClassNames('mapValueToProps', 'mapped', { - value: ['foo'], - mapValueToProps: () => ({ className: 'mapped' }), - }) - - itAppliesProps( - 'mapValueToProps', - { 'data-prop': 'present' }, - { - value: ['foo'], - mapValueToProps: () => ({ 'data-prop': 'present' }), - }, - ) - - itOverridesDefaultProps( - 'mapValueToProps', - { some: 'defaults', overridden: false }, - { some: 'defaults', overridden: true }, - { - value: ['an array'], - mapValueToProps: () => ({ overridden: true }), - }, - ) - - itOverridesDefaultPropsWithFalseyProps('mapValueToProps', { - value: ['an array'], - mapValueToProps: () => ({ undef: undefined, nil: null, zero: 0, empty: '' }), - }) + itReturnsNull(['foo']) + itReturnsNullGivenDefaultProps(['foo']) }) describe('style', () => {