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

Args: Fix issues with string default values #13919

Merged
merged 19 commits into from
Feb 19, 2021

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Feb 16, 2021

Issue: #12071, #12635, #12767

Workaround/fix two issues that led to string default values being summarized as 'string' rather than "'string'", which lead to them being eval-ed as global objects.

Also remove fallback to using the string-ified value of defaults (if in doubt, fallback to undefined and let the true default come through!), and ensure we allow falsey default values.

Add some stories to highlight how we handle various possibilities

`react-docgen-typescript` defaults to setting values to scalars (e.g. `1`, `true`), which is in conflict with `react-docgen` which uses strings (e.g. `"1"`, `"true"`).

This option unifies them, for the most part.
When we pull a default prop summary at runtime, we also need to wrap strings in quotes.
1. Use `Function()` which is more efficient: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#never_use_eval!

2. *DONT* fall back to the stringified version -- this was a workaround for the earlier issues with string values not being wrapped in quotes (in some cases), which was just a bug. Falling back to the string value *only* when the unquoted string didn't exist as a global was a hack.
@tmeasday tmeasday changed the title Better stories for testing defaultValue generation Fix issues with string default values. Issue #12071 Workaround/fix two issues that led to string default values being summarized as 'string' rather than "'string'", which lead to them being eval-ed as global objects. Also remove fallback to using the string-ified value of defaults (if in doubt, fallback to undefined and let the true default come through!). Add some test cases to highlight this stuff. Feb 17, 2021
@tmeasday
Copy link
Member Author

tmeasday commented Feb 17, 2021

@shilman -- I'm not sure if these files are the best place to put these examples? WDYT? (the issue numbers make them a little hard to find, I wonder if a more explicitly named set of "kitchen-sink" examples would help -- maybe they exist already?)

Also do you think I should add one for a class-based component?

// ES6 classes
class Foo extends React.Component {}
Foo.defaultProps = { ...};

// Or old-school
React.createClass({
  getDefaultProps() {
    return { ... }
  }
}

@tmeasday tmeasday changed the title Fix issues with string default values. Issue #12071 Workaround/fix two issues that led to string default values being summarized as 'string' rather than "'string'", which lead to them being eval-ed as global objects. Also remove fallback to using the string-ified value of defaults (if in doubt, fallback to undefined and let the true default come through!). Add some test cases to highlight this stuff. Fix issues with string default values. Feb 17, 2021
@tmeasday tmeasday added the bug label Feb 17, 2021
@tmeasday tmeasday requested a review from shilman February 17, 2021 08:44
@tmeasday tmeasday marked this pull request as ready for review February 17, 2021 08:45
…-defaultValues

Args: Prefer react runtime default values
@shilman shilman changed the title Fix issues with string default values. Args: Fix issues with string default values Feb 17, 2021
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@tmeasday approved all of the null => undefined snapshots. i've left two snapshots unreviewed. can you take a look?

Also there are some broken tests based on your changes.

@shilman
Copy link
Member

shilman commented Feb 18, 2021

@tmeasday restructuring the stories as normal stories makes sense. maybe with a decorator that shows the actual arg values in the preview as well for debugging purposes?

it's been awhile but i think i structured things this way for two reasons:

  1. incorporating examples from issues that don't actually need to be executable in our storybook
  2. providing stories for them AND snapshot tests for them that show both the final output and the intermediate output of docgen, for debugging purposes

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

💯

@anicholls
Copy link

Cross posting for visibility. This PR does not fix #13608 and this issue needs to be reopened. See my latest comment on that issue for more details.

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

Successfully merging this pull request may close these issues.

3 participants