Skip to content

Add support for function paramets without names #218

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

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

odinuge
Copy link
Contributor

@odinuge odinuge commented Sep 24, 2017

Add support for flow functions with parameters that doesn't have a name.
Eg. value: string => void and value: (string, number) => void

This kind of functions ins't currently supported by react-docgen, and it's unable to parse the file.

Source: flow

README.md Outdated
"raw":"string => void",
"signature":{
"arguments":[
{ "name":"_", "type":{ "name":"string" } }
Copy link
Collaborator

@danez danez Oct 5, 2017

Choose a reason for hiding this comment

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

What do you think about instead of using '_' setting the field to null? It seems clearer to me that a consumer of react-docgen checks if the name === null instead of a "random" string name === '_'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @danez,

Got the inspiration from a flow-addon for atom (see screenshot). But I agree that null would be a better and cleaner value for working with the data afterwards. Will update the PR.

screenshot from 2017-10-05 22-16-02

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edit: getNameOrValue returns a string value, and therefore null would break flow. What do you think @danez?

Copy link
Collaborator

@danez danez Oct 5, 2017

Choose a reason for hiding this comment

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

Hmm yes seems it is used quite a bit. We could maybe return an empty string? Then !name would work in the resulting documentation.
My intention is just that we should return something else than a non-empty string, because in the '_' case you would not be able to differ if the variable name was _ or empty.
And consumers could still default to _ in the empty case, if the do not display shorthands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR setting the name to an empty string now.

@@ -25,6 +25,8 @@ export default function getNameOrValue(path: NodePath, raw?: boolean): string {
return node.name;
case types.Literal.name:
return raw ? node.raw : node.value;
case types.FunctionTypeParam.name:
Copy link
Member

Choose a reason for hiding this comment

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

Doing this here seems weird to me. I wouldn't expect getNameOrValue to be called with with a type annotation (but who knows...). It's also not quite right, since some FunctionTypeParam do have a name.

Personally I feel it makes more sense to fix this in getFlowType. If the name of the FunctionTypeParam is null, assign an empty string instead. Something like

name: param.node.name ? param.node.name.name || '';

@danez , why does this use getProperty to extract the name at the moment? Shouldn't it directly by getNameOrValue(param.get('name')) or just param.node.name.name?
Am I looking at the wrong syntax tree or am I missing a case here?

Copy link
Collaborator

@danez danez Oct 11, 2017

Choose a reason for hiding this comment

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

Good question, I have no idea :) But seems unnecessary as it can never be computed or be a literal.

Doing name: param.node.name ? param.node.name.name || ''; should be correct in getFlowType unless the tests disagree.

Add support for flow functions with parameters that doesn't have a name.
Eg. `value: string => void`. It gives the argument a name equal to an
empty string: ''
@odinuge
Copy link
Contributor Author

odinuge commented Oct 11, 2017

Hi @fkling,
After looking further into getFlowType, it makes more sense to do it your way with a simple inline ternary operator, instead of using the getNameOrValue. Updated the PR, and the tests still works as expected. 😄

@fkling
Copy link
Member

fkling commented Oct 11, 2017

Great, thank you all!

@fkling fkling merged commit 0e654af into reactjs:v3-dev Oct 11, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants