eliasm307 / storybook-addon-deep-controls

A Storybook addon that extends @storybook/addon-controls and provides an alternative to interacting with object arguments
MIT License
24 stars 1 forks source link

`userAlreadyDefinedArgTypeForThisPath` returning true unexpecetdly #3

Closed arcticShadow closed 4 months ago

arcticShadow commented 7 months ago

This addons looks like exactly what I need :-D (And exactly what a few people need I suspect)

My use-case, I'm hoping to utilize this to add deep controls for my component props that are typed as a collection of props for another component. A solid use case of this, is I have an icon component, and that has stories And controls as expected to select icons and set rotation etc. For a component that consumes an icon, but allows the consumer to specify the icon. The Icon component prop types are available in full on the ConsumingComponenets Prop types. See the below rough type up representing my use case.

// Icon.tsx
export type IconProps = {icon: string, rotation: Rotation}
export default Icon: FC<{icon: string, rotation: Rotation}> = ({icon, rotation}) => {
   return <i className={`${icon} rotate-${rotation}`></i>
}

// Button.tsx
import {IconProps} from './icon';

export default Button: FC<{icon: IconProps, label: string}> = ({icon, label}) => {
   return (
    <button>
      {icon && <Icon {...icon} /> }
      {label}
    </button>
  );
}

So in adding in deep-controls - I noticed that i'm not getting any deepControls in the argTypes table. I debugged through i bit, and have discovered that the argTypes are being added (enough to progress, but there is an issue here) but when it gets to the userAlreadyDefinedArgTypeForThisPath function - it bails out as it thinks I have explicitly setup the control from my argType and therefore I don't want the deep controls.

I can see what is trying to be achieved however I have not explicitly set my control - My thinking has gone as far as wondering if the detection might be seeing the compiled controls from docgen or similar. I will likely keep looking at this - but for now thought i'd submit this with my use case and findings and potential start a discussion.

arcticShadow commented 7 months ago

Ohh actually I think this addons is expecting the initial ArgType to not have an entry in the context (but due to the use of docgen, an entry does exist)

This touches on the other point i raised above - that only the passed args are being flattened, (which I now see if because it flattens the initial args, not the argType.)

So to achieve my outcomes i'd want to flatten the discovered argType from context (not the initialArgs)

And then i run into this: https://github.com/styleguidist/react-docgen-typescript/issues/487 where docgen isn't outputting the object structure of my referenced types :-/

This might be a loosing battle at the moment.

eliasm307 commented 7 months ago

Hey, thanks for raising this. I'll look into the issue and see if there is a workaround

arcticShadow commented 7 months ago

I carried on down this path for a while longer - but I got stuck in react-docgen-typescript - I cant see an obvious way to patch it to get the type properties out.

So even if we get this intent of this issue resolved, I'll still be a bit stuck with how to achieve my end goal.

But I will keep trying - however for the time being i need to focus on some other issues.

The point of this comment is to say that resolution of this issue is not a high priority (for me)

stevoland commented 5 months ago

Hi, just stumbled on this and am wondering if there is any path forward to this working with argTypes from docgen?

eliasm307 commented 5 months ago

Hi @stevoland, yeah I can look into this if it's a common problem people are having, however would you be able to provide a small reproduction of the issue so I can look into it properly?

This would help a lot as I've not encountered these issues so not sure how my setup is different

stevoland commented 5 months ago

@eliasm307 Sure thing. I'll try to get onto it next week. The package looks really slick btw, thanks!

eliasm307 commented 4 months ago

For anyone having this issue, I'll need a way to recreate it so I can look into it, namely, I'll need atlest the following (with any sensitive information removed):

Providing the code directly in a comment is fine, it doesn't need to be a separate repo/gist

eliasm307 commented 4 months ago

@arcticShadow @stevoland I've released v0.5.0 with some fixes I think are related to this. Please let me know if that solves your issue and if I can close this issue

arcticShadow commented 4 months ago

I have tried out 0.5.0 but still have the same issue.

I have created a reproduction with the two issues (related but probably different) mentioned earlier in this thread https://github.com/arcticShadow/deep-controls-reproduction

The three components

Icon-with-attribute has a prop, with is a type that happens to be the props of icon. this allows any icon props to be specified as an input to icon-with-attribute.

One component (icon-with-attribute) contains stories that have partial deep props - the other component (icon-with-attribute2) is a component that contains no deep props

eliasm307 commented 4 months ago

Hi @arcticShadow, thanks for sending that demo repo. i had some issues getting it running but I've done it now and there are quite a few stories so not sure which one I'm supposed to be looking at, I've looked at the icon-with-attribute1 and icon-with-attribute2 stories and they seem to produce deep controls?

One component (icon-with-attribute) contains stories that have partial deep props

Regarding this comment about partial deep props, I added some notes with the fix for v0.5.0, ie https://github.com/eliasm307/storybook-addon-deep-controls?tab=readme-ov-file#with-the-docs-addon , which says

If you are using the @storybook/addon-docs addon, it will generate controls from the prop types of the component. For this to work properly with deep controls you will need to define explicit initial values for any object properties in args for the corresponding deep controls for those object properties to be added.

ArgTypes added by the docs addon without an initial value are shown with default controls.

ie the deep controls are driven from the args (ie the initial values) not the typescript type definitions for props on the component, so what you need to do is add some default values, e.g. you have the following story:

image

which has controls as follows (ignore iconLocation this comes from the custom argTypes you defined) ie icon.icon is inferred from the initial value:

image

and if I add another property to this e.g. adding the icon title as an empty string

image

then a flattened control is shown for this, ie icon.title:

image

With deep controls disabled, the behaviour is similar, ie you only see the properties of the object that are defined in the args, not all the properties that exist on the typescript type, so hopefully this behaviour isnt surprising

image

Let me know if this was your issue, or if I have misunderstoond something

eliasm307 commented 4 months ago

ive updated the readme to try to be more clear about this

arcticShadow commented 4 months ago

Thanks for updating the readme to clarify.

I think I was hoping for the props to be populated from the ts definitions.

The use case I was holding to fill means I will almost never have a full property definitions (as per the icon examples) but am wanting to expose the full properties, for consumers to be able to "customise" and "build" there own version of the component.

Here's the same icon examples provider earlier, in the context of our design system: https://designsystem.aucklandcouncil.govt.nz/index.html?path=/docs/components-core-icons-icon-attribute--guidelines

If you check out the "component customiser" tab, that's ideally where I'd like to show the full possible properties. (The icon examples are just one of many such places we use this pattern of component props as an input to another component)

eliasm307 commented 4 months ago

Hi @arcticShadow yes i understand your use case, an alternative in your case could be defining the properties you want to expose as argTypes? However this would mean a lot of args types to define and manage

Inferring deep controls from types would be a major enhancement, but maybe it might be possible. However, I think it might not work for some complex cases, eg discriminated type unions where different properties are expected base on the actual values of an object

I'll think about it

arcticShadow commented 4 months ago

I think from memory, my initial exploration into the original problem landed me deep within the upstream typescript docgen parser - where i hit a dead end trying to figure out how to enhance it.

My takeaway was the upstream code needs enhancements, before deep controls would be able to utilise the full type objects dynamically.

As a side note, I'm moving off this project in a couple of weeks so I won't be able to continue using the use case here to help anything progress. Therefore I think it's best to close this ticket as out of scope for the present.

arcticShadow commented 4 months ago

Actually reading through my initial comment in this thread, the original issue is solved.

eliasm307 commented 4 months ago

My takeaway was the upstream code needs enhancements, before deep controls would be able to utilise the full type objects dynamically.

Thats my conclusion also, the docgen seems to just provide the type names but that's not very useful without the type definition, so yeah I would say out of scope for now.

Actually reading through my initial comment in this thread, the original issue is solved.

Cool, in that case I'll just close as completed, the other functionality around generating controls from types is a separate issue

eliasm307 commented 4 months ago

Closed via #6