Skip to content

Determine where children belong using .displayName #143

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Determine where children belong using .displayName #143

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

image

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!

Copy link
Owner

Choose a reason for hiding this comment

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

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).
image

Any chances you used yarn instead of npm? Try running git clean -f -d && rm -rf node_modules && npm i && npm run build.

case DialogTitle.displayName:
titleChildrens.push(child as TitleElement);
return;
case DialogDescription:
case DialogDescription.displayName:
descriptionChildrens.push(child as DescriptionElement);
return;
case DialogButton:
case DialogButton.displayName:
if (Platform.OS === "ios" && buttonChildrens.length > 0) {
buttonChildrens.push(
<View
Expand Down