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.

Sign up for GitHub

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

vue: fix type.name check in extractArgTypes #19956

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9475c1e
vue: fix type.name check in extractArgTypes
ccatterina Nov 24, 2022
5fbdb50
vue: rename `isEnum` and describe its purpose
ccatterina Dec 10, 2022
612eeb8
Merge branch 'next' into norbert/temp2
ndelangen Jan 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions code/renderers/vue/src/docs/extractArgTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import { hasDocgen, extractComponentProps, convert } from '@storybook/docs-tools
const SECTIONS = ['props', 'events', 'slots', 'methods'];

/**
* Check if "@values" tag is defined within docgenInfo.
* If true, then propDef is mutated.
* As @enum tag is not implemented in vuedocgen, infers propdef enum type
* from the presence of @values tag.
*/
function isEnum(propDef: PropDef, docgenInfo: DocgenInfo): false | PropDef {
function inferEnum(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


if (!matched) {
return false;
Expand Down Expand Up @@ -42,7 +42,7 @@ function verifyPropDef(propDef: PropDef, docgenInfo: DocgenInfo): [PropDef, bool

// another callback can be added here.
// callback is mutually exclusive from each other.
const callbacks = [isEnum];
const callbacks = [inferEnum];
for (let i = 0, len = callbacks.length; i < len; i += 1) {
const matched = callbacks[i](propDef, docgenInfo);
if (matched) {
Expand Down