Determine where children belong using .displayName #143
Conversation
This fixes a sub-issue raised within mmazzarolo#141: mmazzarolo#141 (comment)
|
(Build is failing because of a TS error) |
|
Ah thanks for letting me know. In my usage I guess I have a modified tsconfig. I'll load this all up in the actual development environment and make the changes there. |
|
@mmazzarolo any ideas what the best approach to fixing this would be? The only thing I've come up with is adding a |
| @@ -62,14 +62,14 @@ const DialogContainer: React.FC<DialogContainerProps> = (props) => { | |||
| const { styles } = useTheme(buildStyles); | |||
| React.Children.forEach(children, (child) => { | |||
| if (typeof child === "object" && child !== null && "type" in child) { | |||
| switch (child.type) { | |||
| case DialogTitle: | |||
| switch (child.type.displayName) { | |||
There was a problem hiding this comment.
| switch (child.type.displayName) { | |
| // @ts-expect-error: "Property 'displayName' does not exist on type 'string" | |
| const displayName = child.type?.displayName || child.type?.name; | |
| switch (displayName) { |
There was a problem hiding this comment.
Because of the weird situation I'm in with usage of react-native-dialog (and because the build failed) I didn't install that exact version, but instead copied those changes into our internal version and it did work as expected with your code; huzzah! I assume the rationale is to try to fallback to the component's name so that the user doesn't need to manually add a displayName.
I think the child.type should always be readable, because a string should never actually end up as a direct child of Container, right? So in that case the optional chaining shouldn't be necessary if I'm not mistaken. It doesn't affect the TypeScript error. I'm not really sure how type DialogContainerProps = PropsWithChildren<{}> works, but I guess ideally we'd let TypeScript know that the only valid children here are ReactElement and never string. Any thoughts on how to approach that? Or if it would be too convoluted, we could just stick with that ts-ignore comment.
Unfortunately when I'm trying to run the build script on my end, I'm getting additional TypeScript errors I don't recall having seen when trying to build previously, but nothing has changed that I can see. I even rolled back to the latest commit on master (0a9366b65f5b57a268a359604445406e93af1876) but I still get the issues. Maybe I'm just having an incorrect memory here and these errors always occurred for me?
I know you're not able to get React Native running yet, but can you confirm if you're seeing these issues with the build script?
PS: Getting React Native running is such a pain!
There was a problem hiding this comment.
Sorry for the late reply on my end 😞
I assume the rationale is to try to fallback to the component's name so that the user doesn't need to manually add a displayName.
Yep!
I think the child.type should always be readable, because a string should never actually end up as a direct child of Container, right?
I think so. Good call.
Or if it would be too convoluted, we could just stick with that ts-ignore comment.
I'm fine with the @ts-ignore 👍 (Unless you're able to fix it on the prop side, which would be even better (but I'm not sure how to do it properly honesty).
I know you're not able to get React Native running yet, but can you confirm if you're seeing these issues with the build script?
Both this branch and the master one work for me (and work in CI too).
Any chances you used yarn instead of npm? Try running git clean -f -d && rm -rf node_modules && npm i && npm run build.
|
Closed in favor of #150. |
Overview
This fixes a sub-issue raised within #141: #141 (comment)
Test Plan
Changes were verified in our internal codebase. These lines were added to a PR, but not tested separately/a second time after being added to the PR.