-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(factories): remove circular references #550
Conversation
Current coverage is 98.86% (diff: 100%)@@ master #550 diff @@
==========================================
Files 108 108
Lines 1762 1766 +4
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1742 1746 +4
Misses 20 20
Partials 0 0
|
bb23836
to
d72e82c
Compare
@@ -18,7 +18,7 @@ function BreadcrumbDivider(props) { | |||
const classes = cx(className, 'divider') | |||
const ElementType = getElementType(BreadcrumbDivider, props) | |||
|
|||
if (icon) return createIcon(icon, { ...rest, className: classes }) | |||
if (icon) return Icon.create(icon, { ...rest, className: classes }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example static shorthand create method for the Icon. I think this cohesion makes more sense anyhow. Keeping the mapValueToProps
method in the same file as the component that it is creating.
@@ -250,4 +251,6 @@ Button.defaultProps = { | |||
as: 'button', | |||
} | |||
|
|||
Button.create = createShorthandFactory(Button, value => ({ content: value })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example definition of a static shorthand create()
method for our components.
This is where we could pull out the mapValueToProps
method and also make it a static on the Button (see TODO 3 in PR description):
Button.mapValueToProps = value => ({ content: value })
Button.create = createShorthandFactory(Button, Button.mapValueToProps)
@@ -50,21 +46,19 @@ export function createShorthand(Component, mapValueToProps, val, defaultProps = | |||
|
|||
// Map values to props and create a ReactElement | |||
if (_.isString(val) || _.isNumber(val)) { | |||
return <Component {...mergePropsAndClassName(defaultProps, mapValueToProps(val))} /> | |||
return <Component {...mergePropsAndClassName(mapValueToProps(val), defaultProps)} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcarbo I did in fact have to reverse the props for the string/number literal case only. Otherwise, the default type
could not be overridden for createHTMLInput
.
createHTMLInput(this.props.input, { type: 'password' })
In this case, if the user does not provide an input
prop, then we want type='password'
. However, if the mapValueToProps
argument always wins, then the type: 'password'
will always be overridden with type: undefined
.
This is true for only for the literal value usage. The mapValueToProps
is really like a baseProps
, and is only invoked for the literal value case. It should also have less priority than the defaultProps
, which then have less priority than the user's prop object/element props. Though, in this condition there are no user props, just a value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to get some test coverage on these functions. I think that would help make it more clear
I still don't see why we'd need to switch the order in this case. Using your example:
// Given this definition taken from the Input-v1 PR
const createHTMLInput = createShorthandFactory('input', value => ({ type: value }))
// and this usage within the stardust repo
createHTMLInput(this.props.input, { type: 'password' })
// Usage by a consumer of this lib:
<Input input='text' />
// The shorthand would always yield
<input type='password' />
I think about it this way, you can pass props as an object or you can pass a literal that's turned into an object. Both of those cases should be treated the same:
- as an object:
{ type: 'text' }
- as a literal:
text
is turned into{type: 'text'}
I can't how those should be treated differently in terms of overwriting defaultProps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed I have a branch adding tests currently, will push soon. This came up from the Input test for spreads input type
failing since it could no longer override the type. Again, tests are very much needed and will no doubt solve all this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify my point further, you should be able to rewrite this:
export function createShorthand(Component, mapValueToProps, val, defaultProps = {}) {
// Clone ReactElements
if (isValidElement(val)) {
return React.cloneElement(val, mergePropsAndClassName(defaultProps, val.props))
}
// Create ReactElements from props objects
if (_.isPlainObject(val)) {
return <Component {...mergePropsAndClassName(defaultProps, val)} />
}
// Map values to props and create a ReactElement
if (_.isString(val) || _.isNumber(val)) {
return <Component {...mergePropsAndClassName(defaultProps, mapValueToProps(val))} />
}
// Otherwise null
return null
}
...as this:
export function createShorthand(Component, mapValueToProps, val, defaultProps = {}) {
// Clone ReactElements
if (isValidElement(val)) {
return React.cloneElement(val, mergePropsAndClassName(defaultProps, val.props))
}
// Map values to props if it's a literal
if (_.isString(val) || _.isNumber(val)) {
val = mapValueToProps(val)
}
// Create ReactElements from props objects
if (_.isPlainObject(val)) {
return <Component {...mergePropsAndClassName(defaultProps, val)} />
}
// Otherwise null
return null
}
In other words, mapValueToProps is just a convenience that converts a literal to a props object on the end-user's behalf. So when instantiating the component it makes no difference whether the user passed their props as an object or as a literal, they should always win.
d72e82c
to
c5818d9
Compare
The Input PR #281 revealed a circular reference in factories. Button imports factories (to use icon/label). Input imports factories for icon, but also imports Button (for actions).
To avoid circular references and the undefined imports they create, factories for our components have moved to a static create method:
Factories for HTML components do not have circular reference issues since they do not need to import anything. These remain in
factories.js
:TODO
/cc @jcarbo @layershifter, I may do these as well in this PR. Feedback?
createShorthand()
call can be turned into a staticcreate()
method on the component it is creating. This would further standardize things for us.src/lib
. This would further simplify dev "everything lives in /lib"mapValueToProps
on each component. This would make testing easier by being able to reference a single function as the SSOT for how a component chooses to map a value to it's props. Currently, we repeat themapValueToProps
function when we write a test for a shorthand prop. Common tests for Icon/Label/etc also repeat thosemapValueToProps
functions.