-
Notifications
You must be signed in to change notification settings - Fork 165
ISSUE-1463: Fixed undefined prop type error #1696
Conversation
c55554b
to
32ff12f
Compare
@@ -0,0 +1,124 @@ | |||
/* eslint-disable import/no-extraneous-dependencies */ |
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.
Not sure whats up with this file, looks like this wasn't meant to be checked into version control
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.
Whoops! looks like that was left over from my diff, f85e7b2
} else { | ||
typeName = 'number'; | ||
} | ||
typeName = 'enum'; |
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.
Ohh I think I miss understood what you meant when you had asked me about this change. enum
would not be the correct prop type in most cases, I thought you meant you would parse the first value to determine the type. Looking in the terra-repos,oneOf
is usually an enum of strings
options... so this maps to a string prop essentially.
If we only indicate an enum
, this would still require one to look in the code to verify the enum options.
Additionally, to me this indicated a more 'rigid' prop value than what will be required by most props.
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.
What was the reason this was displayed as undefined? I know this occurs because we want to pass Object.keys(DEFAULTS)
, but what information is given as the type 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.
If we only indicate an enum, this would still require one to look in the code to verify the enum options.
Listing the available enum keys in the code comment above the prop that generates the doc could help with this
Additionally, to me this indicated a more 'rigid' prop value than what will be required by most props.
In previous meetings we've talked about not using enums as propTypes but still exporting them so people could use them if they wanted. For example, in the card props image, we'd make the propType a string and list out the values we accept while also exporting an enum that's values were strings. Users could type the string directly or use the enum.
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.
I've dug into the react-docgen code a bit more and it looks like they may handle the case #1463 was logged for.
We've opted to do custom type checking, so we miss out on this feature from react-docgen, but I'd recommend taking a look at this PR and this PR which seem to have added support in react-docgen for resolving Object.keys()
in PropType.oneOf()
and potentially resolving spreads. Here is a util they've made to help resolve enums.
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.
What was the reason this was displayed as undefined
This was displaying as undefined because when we would try to examine typeof type.value[0].value
, we would in fact be trying to call typeof
on Object.values(<whatever the name of your enumerated object was>)
, rather than on [/*the actual array that Object.values() returns */]
– essentially, we'd be making the call before the Object.values
was evaluated, and hence before the objects values were converted into an array. So trying to access the first item in type.value
and look at it's value
would return undefined
, so typeName
was inadvertently getting set to undefined
here.
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.
@dylan-sftwr @bjankord Ah I see what you are saying the issue is. ok. LGTM.
Summary
Removed the ability to auto-infer types of
enum
-type objects in theterra-prop-table
as we're not always able to do so reliably.Fixes #1463
Before:
After:
Additional Details
This change was cleared by @bjankord and @emilyrohrbough
Thanks for contributing to Terra.
@cerner/terra