Resolve issue with first displayName always being used #449
Conversation
| const locals = Array.from( | ||
| (source as any).locals as [string, ts.Symbol][] | ||
| ); | ||
| const hasOneLocalExport = | ||
| locals.filter(local => !!local[1].exports).length === 1; |
There was a problem hiding this comment.
I wasn't certain about this, but it did seem to resolve the one failing test I was dealing with.
Specifically: 'should be taken from stateless component displayName property (using default export) even if file name doesn't match'
There was a problem hiding this comment.
@pvasek @jesstelford curious on any input here
There was a problem hiding this comment.
This produced false-negatives in my scenario. I presume because there is only one with displayName, it was ignoring the others.
There was a problem hiding this comment.
@AMoo-Miki If you log exports there, what's the output?
|
Also, @unicornware since I saw you depending on these changes in flex-development/vite-plugin-react-docgen-typescript#4, I just wanted you to be made aware of this PR. |
## Description Allow multiple exports in components. Most radix components will look like `export const Tooltip = TooltipPrimitive.Root;` The others would be a simpe wrappers over radix with just type changes like like `Omit<TriggerProps, "className"> It's too much to create for each sucj export a standalone file. After this PR following will work. ```ts import * as TooltipPrimitive from "@radix-ui/react-tooltip"; export const Tooltip = TooltipPrimitive.Root; export const TooltipTrigger = TooltipPrimitive.Trigger; ``` Generated exports in case of multiple entries are ``` export propsTooltip: ... export propsTooltipTrigger .... ``` In case of single export previous behaviour is preserved for now, but should be changed IMO in the future PRs Known bugs Until this merged we should not use displayName (have no idea why we use it at all) in files with multiple exports styleguidist/react-docgen-typescript#449 As of now will add comments ``` import * as TooltipPrimitive from "@radix-ui/react-tooltip"; export const Tooltip = TooltipPrimitive.Root; // We can't use .displayName until this is merged styleguidist/react-docgen-typescript#449 // Tooltip.displayName = "Tooltip"; export const TooltipTrigger = TooltipPrimitive.Trigger; // We can't use .displayName until this is merged styleguidist/react-docgen-typescript#449 // TooltipTrigger.displayName = "TooltipTrigger"; ``` ## Steps for reproduction 1. click button 2. expect xyz ## Code Review - [ ] hi @kof, I need you to do - conceptual review (architecture, feature-correctness) - detailed review (read every line) - test it on preview ## Before requesting a review - [ ] made a self-review - [ ] added inline comments where things may be not obvious (the "why", not "what") ## Before merging - [ ] tested locally and on preview environment (preview dev login: 5de6) - [ ] updated [test cases](https://github.com/webstudio-is/webstudio-builder/blob/main/apps/builder/docs/test-cases.md) document - [ ] added tests - [ ] if any new env variables are added, added them to `.env.example` and the `builder/env-check.js` if mandatory
|
There was no activity for a long time. The PR will be closed soon. |
|
Go away, bot. |
|
would love to see these fixed merged in 🙏 |
|
@pvasek let me know if there's anything more you'd like to have done here! |
Fixes: #395
Fixes: #437
Redo of #441
I basically did the fix outlined in #437 by @jesstelford and added some tests.
✅ Tests pass now 🥳
I also made it so that CI runs on each PR push.