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

perf(props): Remove propTypes from production build #731

Merged
merged 11 commits into from
Jan 13, 2017
10 changes: 9 additions & 1 deletion .babelrc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,15 @@
"stage-1"
],
"plugins": [
"lodash"
"lodash",
"transform-react-handled-props",
["transform-react-remove-prop-types", {
"mode": "wrap"
}],
["transform-runtime", {
"polyfill": false,
"regenerator": false
}]
],
"env": {
"test": {
Expand Down
24 changes: 19 additions & 5 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class Dropdown extends Component {

### Define _meta

Every component has a static property called `_meta`. This object defines the component. The values here are used in `propTypes`, generated documentation, generated test cases, and some utilities.
Every component has a static property called `_meta`. This object defines the component. The values here are used for generated documentation, generated test cases and some utilities.

Here's an example `_meta` object:

Expand All @@ -136,9 +136,6 @@ import { META } from '../../lib'
const _meta = {
name: 'MyComponent',
type: META.TYPES.MODULE,
props: {
pointing: ['bottom left', 'bottom right'],
},
}
```

Expand All @@ -162,6 +159,23 @@ class MyComponent {
}
```

### Using propTypes

Every component must have fully described `propTypes`.

```js
import React, { PropTypes } from 'react'

function MyComponent(props) {
return <div className={props.position}>{props.children}</div>
}

MyComponent.propTypes = {
children: PropTypes.node,
position: PropTypes.oneOf(['left', 'right']),
}
```

### Conformance Test

Review [common tests](#common-tests) below. You should now add the [`isConformant()`](#isconformant-required) common test and get it to pass. This will validate the `_meta` and help you get your component off the ground.
Expand All @@ -176,7 +190,7 @@ This will also help with getting early feedback and smaller faster iterations on

Review the SUI documentation for the component. Spec out the component's proposed API. The spec should demonstrate how your component's API will support all the native SUI features. You can reference this [API proposal][7] for the Input.

Once we have solidified the component spec, it's time to write some code. The following sections cover everything you'll need to spec and build your awesome component.
Once we have solidified the component spec, it's time to write some code. The following sections cover everything you'll need to spec and build your awesome component.

## API

Expand Down
82 changes: 5 additions & 77 deletions docs/app/Components/ComponentDoc/ComponentProps.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@ import React, { Component, PropTypes } from 'react'

import { Label, Table } from 'src'

const descriptionExtraStyle = {
fontSize: '0.95em',
color: '#777',
}

/**
* Displays a table of a Component's PropTypes.
*/
Expand All @@ -25,26 +20,9 @@ export default class ComponentProps extends Component {
meta: PropTypes.object,
}

state = {
showEnumsFor: {},
}

toggleEnumsFor = (prop) => () => {
this.setState({
showEnumsFor: {
...this.state.showEnumsFor,
[prop]: !this.state.showEnumsFor[prop],
},
})
}

renderName = (item) => <code>{item.name}</code>
renderName = item => <code>{item.name}</code>

requiredRenderer = (item) => {
if (!item.required) return null

return <Label size='mini' color='red' circular>required</Label>
}
requiredRenderer = item => item.required && <Label size='mini' color='red' circular>required</Label>

renderDefaultValue = (item) => {
let defaultValue = _.get(item, 'defaultValue.value')
Copy link
Member

Choose a reason for hiding this comment

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

Bad merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

^

Expand All @@ -62,52 +40,6 @@ export default class ComponentProps extends Component {
)
}

renderEnums = (item) => {
const { showEnumsFor } = this.state
const { meta } = this.props

if (item.type.indexOf('enum') === -1) return null

const values = meta.props[item.name]
const truncateAt = 30

if (!values) return null

// show all if there are few
if (values.length < truncateAt) {
return (
<p style={descriptionExtraStyle}>
<strong>Enums: </strong>
{values.join(', ')}
</p>
)
}

// add button to show more when there are many values and it is not toggled
if (!showEnumsFor[item.name]) {
return (
<p style={descriptionExtraStyle}>
<strong>Enums: </strong>
<a style={{ cursor: 'pointer' }} onClick={this.toggleEnumsFor(item.name)}>
Show all {values.length}
</a>
<div>{values.slice(0, truncateAt - 1).join(', ')}...</div>
</p>
)
}

// add "show more" button when there are many
return (
<p style={descriptionExtraStyle}>
<strong>Enums: </strong>
<a style={{ cursor: 'pointer' }} onClick={this.toggleEnumsFor(item.name)}>
Show less
</a>
<div>{values.join(', ')}</div>
</p>
)
}

render() {
const { props: propsDefinition } = this.props
const content = _.sortBy(_.map(propsDefinition, (config, name) => {
Expand All @@ -129,6 +61,7 @@ export default class ComponentProps extends Component {
description: description && description.split('\n').map(l => ([l, <br key={l} />])),
}
}), 'name')

return (
Copy link
Member

Choose a reason for hiding this comment

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

Bad merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

^

<Table data={content} className='very basic compact'>
<Table.Header>
Expand All @@ -145,14 +78,9 @@ export default class ComponentProps extends Component {
<Table.Row key={item.name}>
<Table.Cell>{this.renderName(item)}</Table.Cell>
<Table.Cell>{this.requiredRenderer(item)}</Table.Cell>
<Table.Cell>
{item.type}
</Table.Cell>
<Table.Cell>{item.type}</Table.Cell>
<Table.Cell>{this.renderDefaultValue(item.defaultValue)}</Table.Cell>
<Table.Cell>
{item.description && <p>{item.description}</p>}
{this.renderEnums(item)}
</Table.Cell>
<Table.Cell>{item.description && <p>{item.description}</p>}</Table.Cell>
</Table.Row>
))}
</Table.Body>
Expand Down
5 changes: 5 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@
"babel-loader": "^6.2.8",
"babel-plugin-__coverage__": "^11.0.0",
"babel-plugin-lodash": "^3.2.10",
"babel-plugin-react-transform": "^2.0.2",
"babel-plugin-transform-react-constant-elements": "^6.9.1",
"babel-plugin-transform-react-handled-props": "^0.2.1",
"babel-plugin-transform-react-remove-prop-types": "^0.2.10",
"babel-plugin-transform-runtime": "^6.15.0",
"babel-preset-es2015": "^6.18.0",
"babel-preset-react": "^6.5.0",
"babel-preset-stage-1": "^6.5.0",
Expand Down
11 changes: 3 additions & 8 deletions src/elements/Rail/Rail.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,6 @@ function Rail(props) {
Rail._meta = {
name: 'Rail',
type: META.TYPES.ELEMENT,
props: {
close: ['very'],
position: SUI.FLOATS,
size: _.without(SUI.SIZES, 'medium'),
},
}

Rail.propTypes = {
Expand All @@ -70,7 +65,7 @@ Rail.propTypes = {
/** A rail can appear closer to the main viewport. */
close: PropTypes.oneOfType([
PropTypes.bool,
PropTypes.oneOf(Rail._meta.props.close),
PropTypes.oneOf(['very']),
]),

/** A rail can create a division between itself and a container. */
Expand All @@ -80,10 +75,10 @@ Rail.propTypes = {
internal: PropTypes.bool,

/** A rail can be presented on the left or right side of a container. */
position: PropTypes.oneOf(Rail._meta.props.position).isRequired,
position: PropTypes.oneOf(SUI.FLOATS).isRequired,

/** A rail can have different sizes. */
size: PropTypes.oneOf(Rail._meta.props.size),
size: PropTypes.oneOf(_.without(SUI.SIZES, 'medium')),
}

export default Rail
31 changes: 2 additions & 29 deletions src/lib/getUnhandledProps.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,3 @@
/**
* Push all `source` array elements to the `target` array if they don't already exist in `target`.
*
* @param {Array} source - An array of elements to add to the `target`
* @param {Array} target - An array to receive unique elements from the `source`
* @returns {Array} Mutated `target` array
*/
const pushUnique = (source, target) => source.forEach(x => {
if (target.indexOf(x) === -1) target.push(x)
})

/**
* Returns an object consisting of props beyond the scope of the Component.
* Useful for getting and spreading unknown props from the user.
Expand All @@ -17,25 +6,9 @@ const pushUnique = (source, target) => source.forEach(x => {
* @returns {{}} A shallow copy of the prop object
*/
const getUnhandledProps = (Component, props) => {
const { autoControlledProps, defaultProps, propTypes } = Component
let { handledProps } = Component

// ----------------------------------------
// Calculate handledProps once and cache
// ----------------------------------------
if (!handledProps) {
handledProps = []

if (autoControlledProps) pushUnique(autoControlledProps, handledProps)
if (defaultProps) pushUnique(Object.keys(defaultProps), handledProps)
if (propTypes) pushUnique(Object.keys(propTypes), handledProps)

Component.handledProps = handledProps
}
// Note that `handledProps` are generated automatically during build with `babel-plugin-transform-react-handled-props`
const { handledProps = [] } = Component
Copy link
Member

Choose a reason for hiding this comment

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

How about an inline comment here noting that this is added by the plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I'll add comment immediately.


// ----------------------------------------
// Return _unhandled_ props
// ----------------------------------------
return Object.keys(props).reduce((acc, prop) => {
if (prop === 'childKey') return acc
if (handledProps.indexOf(prop) === -1) acc[prop] = props[prop]
Expand Down
36 changes: 21 additions & 15 deletions test/specs/commonTests.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,27 @@ export const isConformant = (Component, options = {}) => {
})
})

describe('handles props', () => {
it('defines handled props in Component.handledProps', () => {
Component.should.have.any.keys('handledProps')
Component.handledProps.should.be.an('array')
})

it('Component.handledProps includes all handled props', () => {
const computedProps = _.union(
Component.autoControlledProps,
_.keys(Component.defaultProps),
_.keys(Component.propTypes),
)
const expectedProps = _.uniq(computedProps).sort()

Component.handledProps.should.to.deep.equal(expectedProps,
'It seems that not all props were defined in Component.handledProps, you need to check that they are equal ' +
'to the union of Component.autoControlledProps and keys of Component.defaultProps and Component.propTypes'
)
})
})

// ----------------------------------------
// Events
// ----------------------------------------
Expand Down Expand Up @@ -460,15 +481,6 @@ export const rendersChildren = (Component, options = {}) => {
// ----------------------------------------
// className from prop
// ----------------------------------------
const _definesPropOptions = (Component, propKey) => {
it(`defines ${propKey} options in Component._meta.props`, () => {
Component.should.have.any.keys('_meta')
Component._meta.should.have.any.keys('props')
Component._meta.props.should.have.any.keys(propKey)
Component._meta.props[propKey].should.be.an('array')
})
}

const _noDefaultClassNameFromProp = (Component, propKey, options = {}) => {
const { className = propKey, requiredProps = {} } = options
// required props may include a prop that creates a className
Expand Down Expand Up @@ -592,7 +604,6 @@ export const implementsWidthProp = (Component, options = {}) => {
describe(`${propKey} (common)`, () => {
assertRequired(Component, 'a `Component`')

_definesPropOptions(Component, propKey)
_noDefaultClassNameFromProp(Component, propKey, options)
_noClassNameFromBoolProps(Component, propKey, options)

Expand Down Expand Up @@ -853,7 +864,6 @@ export const implementsTextAlignProp = (Component, options = {}) => {
describe('aligned (common)', () => {
assertRequired(Component, 'a `Component`')

_definesPropOptions(Component, 'textAlign')
_noDefaultClassNameFromProp(Component, 'textAlign', options)
_noClassNameFromBoolProps(Component, 'textAlign', options)

Expand Down Expand Up @@ -889,7 +899,6 @@ export const implementsVerticalAlignProp = (Component, options = {}) => {
describe('verticalAlign (common)', () => {
assertRequired(Component, 'a `Component`')

_definesPropOptions(Component, 'verticalAlign')
_noDefaultClassNameFromProp(Component, 'verticalAlign', options)
_noClassNameFromBoolProps(Component, 'verticalAlign', options)

Expand Down Expand Up @@ -952,7 +961,6 @@ export const propValueOnlyToClassName = (Component, propKey, options = {}) => {
assertRequired(Component, 'a `Component`')
assertRequired(propKey, 'a `propKey`')

_definesPropOptions(Component, propKey)
_noDefaultClassNameFromProp(Component, propKey, options)
_noClassNameFromBoolProps(Component, propKey, options)

Expand Down Expand Up @@ -990,7 +998,6 @@ export const propKeyAndValueToClassName = (Component, propKey, options = {}) =>
assertRequired(Component, 'a `Component`')
assertRequired(propKey, 'a `propKey`')

_definesPropOptions(Component, propKey)
_noDefaultClassNameFromProp(Component, propKey, options)
_noClassNameFromBoolProps(Component, propKey, options)
_classNamePropValueBeforePropName(Component, propKey, options)
Expand All @@ -1013,7 +1020,6 @@ export const propKeyOrValueAndKeyToClassName = (Component, propKey, options = {}
assertRequired(Component, 'a `Component`')
assertRequired(propKey, 'a `propKey`')

_definesPropOptions(Component, propKey)
_noDefaultClassNameFromProp(Component, propKey, options)
_classNamePropValueBeforePropName(Component, propKey, options)
beforeEach(() => {
Expand Down
Loading