react-docgen-typescript icon indicating copy to clipboard operation
react-docgen-typescript copied to clipboard

feat: Export raw expression when shouldIncludeExpression is set

Open TrevorBurnham opened this issue 2 years ago • 13 comments

Resolves #432 cc @joshwooding

This PR adds a property called rawExpression to the parsed object when shouldIncludeExpression is set, in addition to the existing expression property. The two are identical in many cases, but diverge when the exported component has a wrapper such as React.memo. Example from the test cases:

// parsed.expression
function Jumbotron(props: JumbotronProps) {
  return <div>Test</div>;
}

// parsed.rawExpression
export default React.memo(Jumbotron);

Exporting both will allow developers to extract additional information from the raw expression when necessary.

TrevorBurnham avatar May 09 '22 01:05 TrevorBurnham

@pvasek Sorry to bother you, do you know if we can merge this and do a release?

joshwooding avatar May 11 '22 15:05 joshwooding

I understand the problem you are trying to solve. I think it's a little bit harder to know if that's (#433) really a bug or not. I mean who knows how the expression is really used? So I understand your solution will not break anyone's code (most probably 🙂)

The only thing that worries me is that we are expanding public API and what if we find there is a similar issue somewhere else and we would end up with something like rootRawEpxression or something similar.

It would be great to hear someone who is using that expression property already and what are their takeaways.

pvasek avatar May 12 '22 19:05 pvasek

I understand the problem you are trying to solve. I think it's a little bit harder to know if that's (#433) really a bug or not. I mean who knows how the expression is really used? So I understand your solution will not break anyone's code (most probably 🙂)

The only thing that worries me is that we are expanding public API and what if we find there is a similar issue somewhere else and we would end up with something like rootRawEpxression or something similar.

It would be great to hear someone who is using that expression property already and what are their takeaways.

That's a good point. I don't really expect many people to be using the expression property. I have an example of using the rawExpression property though: https://github.com/joshwooding/vite-plugin-react-docgen-typescript/pull/3/files#diff-18112469e0b5ce0660c8b3e9bbdf6d1baf6099883d45a75ea2f8ac502c90af3dR22 which in my opinion is what the expression property was trying to solve initially.

joshwooding avatar May 12 '22 20:05 joshwooding

I don't really expect many people to be using the expression property. I have an example of using the rawExpression property though…

Hmm. That example uses rawExpression to get the component identifier, but I think there are cases where expression is actually better suited to that purpose, as seen in the test case here… right?

Since getting the component identifier is the main reason expression was added, and also the main reason why rawExpression was requested, the ideal solution here would probably be to export the component identifier directly, rather than pushing that work onto consumers like react-docgen-typescript-plugin and vite-plugin-react-docgen-typescript.

I see one notable use case for exporting the expression, though, and that's for the case where the component has no identifier (for example, export default function() { /* component code */ }). In that case the downstream would need to modify the AST in order to add an identifier (e.g. transforming the above to export default function asd1234() { /* component code */ }). For that particular example, expression and rawExpression are identical, so there may not be a need to export both? Though maybe there's a variant on the case that I'm not thinking of.

TrevorBurnham avatar May 12 '22 21:05 TrevorBurnham

I don't really expect many people to be using the expression property. I have an example of using the rawExpression property though…

Hmm. That example uses rawExpression to get the component identifier, but I think there are cases where expression is actually better suited to that purpose, as seen in the test case here… right?

Since getting the component identifier is the main reason expression was added, and also the main reason why rawExpression was requested, the ideal solution here would probably be to export the component identifier directly, rather than pushing that work onto consumers like react-docgen-typescript-plugin and vite-plugin-react-docgen-typescript.

I see one notable use case for exporting the expression, though, and that's for the case where the component has no identifier (for example, export default function() { /* component code */ }). In that case the downstream would need to modify the AST in order to add an identifier (e.g. transforming the above to export default function asd1234() { /* component code */ }). For that particular example, expression and rawExpression are identical, so there may not be a need to export both? Though maybe there's a variant on the case that I'm not thinking of.

expression in the test case equals FunctionComponent which is the type of the function and not really usable whilst the rawExpression is the default export. I guess returning the identifier is also a suitable solution too though.

joshwooding avatar May 12 '22 22:05 joshwooding

Thank you for explaining these details to me.

If we really need both could we come up with a better name than rawExpression? Something that would explain what to really expect there?

pvasek avatar May 13 '22 05:05 pvasek

Thank you for explaining these details to me.

If we really need both could we come up with a better name than rawExpression? Something that would explain what to really expect there?

Do you think rawExpression should be preferred over outputting the identifier?

joshwooding avatar May 13 '22 08:05 joshwooding

Any update on this?

joshwooding avatar May 22 '22 16:05 joshwooding

Thank you for explaining these details to me. If we really need both could we come up with a better name than rawExpression? Something that would explain what to really expect there?

Do you think rawExpression should be preferred over outputting the identifier?

What should outputting the identifier looks like? How it will be different from rawExpression?

If I understand it correctly we have two distinct options:

  1. rawExpression preferably with better name
  2. outputting the identifier, whatever it means

pvasek avatar May 22 '22 19:05 pvasek

Thank you for explaining these details to me. If we really need both could we come up with a better name than rawExpression? Something that would explain what to really expect there?

Do you think rawExpression should be preferred over outputting the identifier?

What should outputting the identifier looks like? How it will be different from rawExpression?

If I understand it correctly we have two distinct options:

  1. rawExpression preferably with better name
  2. outputting the identifier, whatever it means

The identifier would just be the variable or function name of the declaration. This can be computed using rawExpression.

  1. I guess something like rootExpression or even replacing expression would be fine to me.
  2. This is the option that doesn't leak internals but is less flexible.

joshwooding avatar May 22 '22 19:05 joshwooding

@TrevorBurnham What do you think about changing rawExpression to rootExpression?

pvasek avatar May 28 '22 09:05 pvasek

@TrevorBurnham any update on this?

joshwooding avatar Jun 24 '22 12:06 joshwooding

@pvasek @joshwooding Sorry for the delay in getting back to you. As suggested, I've changed rawExpression to rootExpression here. 👍


I do think this is something of a workaround rather than an ideal patch. To reiterate my comment above:

the ideal solution here would probably be to export the component identifier directly, rather than pushing that work onto consumers like react-docgen-typescript-plugin and vite-plugin-react-docgen-typescript

That would mean implementing an algorithm that can extract the identifier from components no matter how they're defined. So identifier would be Rosebud for all of these scenarios (just to name a few):

export const Rosebud = () => {};
export function Rosebud() {};
export class Rosebud extends React.Component {};
const Rosebud = () => {};
export default Rosebud;
export const Rosebud = hoc(Sled);

That way, packages like the one I mentioned could use the identifier to generate code like Rosebud.displayName = 'Rosebud'.

TrevorBurnham avatar Jun 25 '22 03:06 TrevorBurnham