[code-infra] Upgrade react-docgen to v8 #47685
Conversation
JCQuintas
added
type: enhancement
Netlify deploy previewhttps://deploy-preview-47685--material-ui.netlify.app/ Bundle size report
|
|
@mui/infra This is currently working, we might want to divide some of the changes to isolate them, as I took some liberty in the path I took. I want to hear your opinion on this though, before I go further. My main motivation was to fix an issue with |
| distinctDefinitions.set(definition.$$id, definition); | ||
| } | ||
| // Always update so that the last definition's jsDoc wins | ||
| // This ensures that when types are intersected (A & B), the last type's documentation is used |
There was a problem hiding this comment.
So it went from first to last definition wins?
There was a problem hiding this comment.
Reasoning was more that usually we have
interface A extends B {} which is similar to A & B. It felt a bit arbitrary that the first wins, which would prevent some overriding.
Ideally the best way is to Omit<A, keyof B> & B though
There was a problem hiding this comment.
interface A extends B {}feels similar to
B & Ato me, as in A's props should win over Bs, but that isn't how this code interpretes it?
There was a problem hiding this comment.
True, A should win in the first, but B should in the second. 🫠
I checked it on the playground
Inheritance picks the first time the prop appears 🙃
If you override it, then it ignores any previous declaration.
Intersection merges all declarations together.
There was a problem hiding this comment.
I think ideally we take interface behavior, because that can't simply be altered to fix the comment. i.e.
interface X extends Y, Z {
/** foo */
prop: number
}we can easily move Y and Z around, but we can't really reorder X. even though, if one comment should win, it's probably X here.
For types, we can freely change the order, so it doesn't really matter what we do in terms of "first wins" or "last wins", we can pick what interfaces do and adjust order where needed.
Instead of adding more Omit, maybe it's a matter of reordering the types in the intersection?
There was a problem hiding this comment.
I've reworked it a bit to follow typescript, which required less changes in the end :)
Divided them into commits for each component, each with a vague-ish description of the reason 😆
In a few cases it still made sense to add Omit, else the types (when viewed by the user in a code editor) would still be merged/messy.
|
Took the liberty of removing a few more |
To me, the intuitive thing to do is that the comment on the prop of the overwrite wins, if there is one. Not sure if that's hard todo or not, but with that logic, do we really need to exclude props? |
…st omit same keys
| return declarationsWithComments[0].comment; | ||
| } | ||
|
|
||
| // Multiple declarations with comments - this is the intersection case (type C = A & B) |
There was a problem hiding this comment.
So that means if you change an interface to a type or vice-versa, the extracted comments suddenly can look different?
interface A {
/** foo */
prop: Prop
}
interface B {
/** bar */
prop: Prop
}
type X = A & B & {
/** baz */
prop: Prop
}interface C extends B, A {
/** baz */
prop: Prop
}
type X = CSo these wouldn't produce the same output in the proptypes, right? There seems to be an open issue about this microsoft/TypeScript#30901. Your stance is to try emulate intellisense as closely as possible? I hate this behavior, but it also kind of makes sense to make sure the docs and the intellisense in vscode look the same. Maybe link to that open issue so we can periodically check if it ever gets fixed.
There was a problem hiding this comment.
Yes, changing them could look different, which is the behaviour of the intellisense as you mentioned: https://www.typescriptlang.org/play/?#code/JYOwLgpgTgZghgYwgAgILIN4ChnIPQBUByMA9qcgXjsgA5Sm0BcyIArgLYBG0WAvliyhIsRCgBCmGoWJc4UStVz1GLdt14CsYAJ60UADWQBeNMgBkySZey4ZyOQC9FNFc1aceUfoOHR4SMgAwsgQAB6QIAAmAM5WADRmtvhEDnDOVK4M7upePtp6KACaJsGCCKQgMWDIYSxGpsnK2SwAjD4VVTU6LCWNNM2qyABM-EA
I find the internal behaviour of intellisense weird too, but I think it is sensible that we emulate what the users would actually see in the interface in order to better control it.
Personally I don't look at the libraries docs most of the time. I'm reading the types documentation in either the UI or the declarations, so this is technically a "bugfix" for me 😆
I'll mention the issue in code
Janpot
left a comment
Janpot
left a comment
There was a problem hiding this comment.
Looks good to me, adding @siriwatknp for a review
Did you test how these changes affect X
I did, and they will affect X quite a bit, I was waiting for a clearer direction before opening the PR, I'll link it here when it is done |
|
X changes mui/mui-x#21155 |
3 participants
prop?.that are needed, we weren't taking that they were undefined before, current types should be better as they come from the libButtonBase.d.ts): Added dynamic Omit to exclude props that child types redefinePaper.d.ts): Same pattern to preventchildrenandsxduplication in Accordion, AppBar, etc.List.d.ts): Same pattern to preventchildrenduplication in MenuListstyles/index.d.ts): Removed JSDoc to preventclassesduplicationtransitions/transition.ts): DefinedaddEndListenerlocally with JSDoc to avoid duplication from external react-transition-group typesvariantfrom BaseSelectProps (moved JSDoc to OutlinedSelectProps only)'defaultValue' | 'sx'to Omit in all variant interfaceschildrenprop since it was no longer inheritedSwipeableDrawerSlotsextendDrawerSlotsand used singleCreateSlotsAndSlotPropsto avoid duplicate slots/slotProps JSDocX changes mui/mui-x#21155