Skip to content

handle more complex intersection/union props #247

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 4 commits into from
Apr 25, 2020

Conversation

hipstersmoothie
Copy link
Contributor

@hipstersmoothie hipstersmoothie commented Apr 23, 2020

closes #158
closes #57

Addresses some of #68 but that issue seems to be more about displaying separate props tables for each set of properties.

This pull request aims to better react-docgen-typescript's ability to document complex prop types.

There were a few of these situations fixed in #241 but I missed a few cases.

Case 1:

Intersection + Union

type ExampleProps = { foo: string } & ({ bar: string } | { baz: string });

export const Example = (props: ExampleProps) => <div />;

Previous: Only foo received documentation
Now: All props receive documentation

Case 2:

Discriminated Intersection + Union

type StackProps = { foo: string } & (
  | { bar: 'one'; test: number }
  | { bar: 'other'; baz: number }
);

export const ExampleProps = (props: StackProps) => <div />

Previous: Only foo and bar received documentation
Now: All props receive documentation

Case 3:

Complex Intersection + Union with overlapping props

interface StackBaseProps<T> {
  as: T;
  hasWrap?: boolean;
}

interface StackJustifyProps {
  foo?: 'blue';
  /** You cannot use gap when using a "space" justify property */
  gap?: never;
}

interface StackGapProps {
  foo?: 'red';
  /** The space between children */
  gap?: number;
}

type StackProps<T> = StackBaseProps<T> & (StackGapProps | StackJustifyProps);

export const ComplexGenericUnionIntersection = <T extends any>(
  props: StackProps<T>
) => <div />;

Previous: gap would receive wrong type, hasWrap was required for some reason too. I also found some situations where the union type would be overwritten to be just blue or red
Now: All overlapping receive full merged documentation.

@hipstersmoothie hipstersmoothie marked this pull request as draft April 23, 2020 23:27
@hipstersmoothie
Copy link
Contributor Author

I'm a little unsatisfied with how this turned out. It works but I feel like there is a pretty big room for error.

I manually merge the prop docs we get, and try to combine some unions in a nice way. It works but I think it could be better.

Screen Shot 2020-04-23 at 5 28 21 PM

Things I'm still gonna try:

  1. rely on typescript to create merged definition (tried this already for awhile, it's hard)
  2. see what rending multiple props is like

@hipstersmoothie
Copy link
Contributor Author

2 seems like a breaking change. Currently this library only return 1 set or props per named component.

I tried adding a number to the end of repeated props, looks prettty bad

I tried returning multiple props-tables (wouldn't be a major) but the consuming tool would need to be configured to consumer the extra tables. A caveat here though is that there might be a bunch of extras.

So I don't think that 2 is an option.

I will explore 1 but it is something that could also come at a later date if we find this implementation to be buggy. I think it should be possible, I just wasn't able to bend the TS compiler to my will.... today 😄

@hipstersmoothie hipstersmoothie marked this pull request as ready for review April 24, 2020 23:04
const baseProps = propsType.getProperties();
const propertiesOfProps: ts.Symbol[] = propsType.isUnionOrIntersection()
? // Using internal typescript API to get all properties
(this.checker as any).getAllPossiblePropertiesOfTypes(propsType.types)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first big think I did was use this internal TypeScript compiler API. It is way better at getting all of the types than what I did before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question is how this internal API is supported across the typescript versions. Maybe it would make sense to also include those version as peer dependency for this package. Which is requested in #245 and probably make sense. @hipstersmoothie What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh sorry, didn't noticed #248 before. Does it means that this internal API is there from version 3 onward.

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 checked an it's been present all of v3 https://www.unpkg.com/[email protected]/lib/typescript.js

At @orta's suggestion I opened up an issue to request that this API be made public

!hasCodeBasedDefault &&
// If in a intersection or union check original declaration for "?"
// @ts-ignore
declarations.every((d) => !d.questionToken) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One caveat of the new above approach is that the flags for each prop mark it as required for some reason. To fix this I look at the declarations of the properties (it could be on multiple interfaces) to see if it was marked as optional.

Then on the line below I also check that if there is a property returned from propsType.getProperties(); check if that property is optional. This fixed the test case where we extend from a picked interface.

@hipstersmoothie
Copy link
Contributor Author

I was able to do this in a smarter way using the typescript compiler for the most part. It's way better than the original implementation now! (mostly thanks to the internal API I found)

@pvasek This is ready for review now.

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

Successfully merging this pull request may close these issues.

Intersection types are not documented Support for Union types
2 participants