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

vue: fix type.name check in extractArgTypes #19956

Merged
merged 3 commits into from
Jan 14, 2023
Merged

Conversation

ccatterina
Copy link
Contributor

Hi there,
the changes in 38b2de0 have broken the support @values in args table feature because the isEnum, despite the misleading name, should convert a not enum prop into an enum prop.

@JReinhold
Copy link
Contributor

Any thoughts on this @ProjektGopher ?

@ProjektGopher
Copy link
Contributor

ProjektGopher commented Nov 25, 2022

Changing this equality check will definitely break what had been fixed previously - Are you saying that if I have a jsdoc with @values, that it should be converted to enum only if the type != enum? Because that seems wrong to me.

Can I see the jsdoc that's triggering this? Is it just missing @type enum?

@ccatterina
Copy link
Contributor Author

ccatterina commented Nov 25, 2022

Changing this equality check will definitely break what had been fixed previously

I've also added a condition that checks if type is defined and it is not null to avoid to brake what you had fixed.

Are you saying that if I have a jsdoc with @values, that it should be converted to enum only if the type != enum? Because that seems wrong to me.

After looking at the initial PR that introduced the feature, I supposed that the type enum was not available for the vue components documentation, indeed all the changes #16019 focus on mutating the type of the propdef.

Anyway, i tried to add the tag @type enum but the radio input is still not rendered in the docs page:

     /**
       * Test prop
       * @type enum
       * @values foo, bar
       */
      test: {
        required: false,
        type: String,
      },

it's rendered like this:

image

and this is the result after changing the condition:

image

Looking through the vue docgen documentation it seems that @type tag is not available.

I think that the type.name can be only inferred by the vue prop type and that's why the #16019 introduced this mechanism.

@ProjektGopher
Copy link
Contributor

I've also added a condition that checks if type is defined and it is not null to avoid to brake what you had fixed.

Funny you should mention that, because that's what I had done in my initial PR. It was changed to a nullsafe chain before being merged which is what then surfaced this incorrect equality check.

Does that second render only work like that when the equality is reversed, or does it also work this way once the type check is done separately instead of chaining?

@ccatterina
Copy link
Contributor Author

ccatterina commented Nov 25, 2022

It also works with the type check done separately instead of chaining

@ProjektGopher
Copy link
Contributor

Sounds to me like that's our winning approach

@valentinpalkovic
Copy link
Contributor

@ccatterina, @ProjektGopher Thanks for the contribution. It seems the discussion is ongoing. Let us know, if you figured out a final solution.

@ccatterina
Copy link
Contributor Author

I think this can be merged. Essentially, I repurposed the same code that was originally written by @ProjektGopher in #18710 and that had been modified in a cleanup commit (2f18165) before merging the PR. @ProjektGopher am I right?

@@ -11,7 +11,7 @@ const SECTIONS = ['props', 'events', 'slots', 'methods'];
function isEnum(propDef: PropDef, docgenInfo: DocgenInfo): false | PropDef {
// cast as any, since "values" doesn't exist in DocgenInfo type
const { type, values } = docgenInfo as any;
const matched = Array.isArray(values) && values.length && type?.name === 'enum';
const matched = Array.isArray(values) && values.length && type && type.name !== 'enum';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const matched = Array.isArray(values) && values.length && type && type.name !== 'enum';
const matched = Array.isArray(values) && values.length && type && type.name === 'enum';

This should fix your issue, while leaving my original fix intact

Copy link
Contributor

Choose a reason for hiding this comment

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

In my original PR, that equality check was still wrong, it just didn't surface until the move to optional chaining

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably misunderstood your question

Does that second render only work like that when the equality is reversed, or does it also work this way once the type check is done separately instead of chaining?

The second render works in the following cases:

const matched = Array.isArray(values) && values.length && type?.name !== 'enum';
const matched = Array.isArray(values) && values.length && type && type.name !== 'enum';

but not in case the equality is not reversed:

const matched = Array.isArray(values) && values.length && type && type.name === 'enum';

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok, hmm... I'll have to take another look at this

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @ccatterina , sorry for the late reply - Covid finally made it into our home, so I've been super busy with a sick and preggo wife, and a sick toddler.

If I'm understanding you correctly, the neg type check is so that we're casting anything that behaves like an enum into one? I wonder if adding a small docblock would help clear this up in the future.

I dug into some of these other resources, and it looks like there's specifically an @enum directive that isn't implemented in vuedocgen.

@valentinpalkovic I think I'm going to resolve this conversation as I think the proposed solution is the way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that you two (seemingly smart people!) have discussed how this single character make it all behave, I think it makes sense to refactor the code to be more readable for future work. Either by making the matched variable more verbose, or by adding comments describing what the conditions do, and why.

I don't know much about Vue so I trust that you two have got it under control, but the next person is probably going to be just as confused as you initially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I think that the most confusing thing is the name of the function isEnum, maybe something like inferEnum comes out clearer.

I could rename the function and a comment to describe its purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JReinhold @ProjektGopher I pushed a new commit that tries to make the purpose of the function clearer, feel free to make some changes (my english is not so good).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this does a decent job explaining the situation. I feel like it's good to go

@ndelangen ndelangen merged commit 6522058 into storybookjs:next Jan 14, 2023
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.

5 participants