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

Fix crashes when DefaultProp isn't a string #1510

Merged
merged 2 commits into from
Jan 6, 2020

Conversation

Lazyuki
Copy link
Contributor

@Lazyuki Lazyuki commented Jan 3, 2020

Styleguidist crashes when default prop isn't a string, e.g. a number.
image

@codecov
Copy link

codecov bot commented Jan 3, 2020

Codecov Report

Merging #1510 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/client/rsg-components/Props/renderDefault.tsx 100% <100%> (ø) ⬆️

Copy link
Member

@sapegin sapegin left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@@ -9,18 +9,19 @@ export default function renderDefault(prop: PropDescriptorWithFlow): React.React
// Workaround for issue https://github.com/reactjs/react-docgen/issues/221
// If prop has defaultValue it can not be required
if (prop.defaultValue) {
const defaultValueString = typeof prop.defaultValue.value === 'string' ? showSpaces(unquote(prop.defaultValue.value)) : showSpaces(String(prop.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.

I guess we don't really need a condition here?

Suggested change
const defaultValueString = typeof prop.defaultValue.value === 'string' ? showSpaces(unquote(prop.defaultValue.value)) : showSpaces(String(prop.defaultValue.value));
const defaultValue = showSpaces(unquote(String(prop.defaultValue.value)));

Copy link
Contributor Author

@Lazyuki Lazyuki Jan 3, 2020

Choose a reason for hiding this comment

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

Yeah that works too. Although I just realized if the defaultValue isn't a primitive type like an object it would probably show [object Object] as the default prop?

EDIT: nvm if it's an object it gets evaluated correctly I guess

@Lazyuki
Copy link
Contributor Author

Lazyuki commented Jan 3, 2020

BTW if this gets merged to master can you also update the 11 branch with another beta release if possible?

@sapegin sapegin merged commit 626e505 into styleguidist:master Jan 6, 2020
@styleguidist-bot
Copy link
Collaborator

🎉 This PR is included in version 10.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Lazyuki
Copy link
Contributor Author

Lazyuki commented Jan 6, 2020

@sapegin sorry to bother you but would it be possible to merge master into 11 and release 11.0.0-beta3? I see that there's another commit on master so I'm not sure if you are willing to merge that just yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants