Skip to content

FieldTemplate children are in an array #1159

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.

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
2 of 3 tasks
mlbstrm opened this issue Jan 29, 2019 · 10 comments
Closed
2 of 3 tasks

FieldTemplate children are in an array #1159

mlbstrm opened this issue Jan 29, 2019 · 10 comments

Comments

@mlbstrm
Copy link

mlbstrm commented Jan 29, 2019

Prerequisites

Description

Since release 1.2.0 (and more specifically #1118), children passed to FieldTemplate are in an array.

Steps to Reproduce

  1. Open the playground on the Simple example.
  2. Using the React Developer Tools, inspect the first FieldTemplate component and its props.

image

Expected behavior

children should be a node.

Actual behavior

children is an Array with 2 extra null items.

Version

1.2.0

@ketysek
Copy link

ketysek commented Apr 9, 2019

Confirming, update to 1.3.0 broked my own field templates, so I reverted back to 1.1.0

@epicfaace
Copy link
Member

Hmm you are right. @ketysek @mlbstrm would any of you be willing to contribute a fix?

@anothertempore
Copy link
Contributor

@epicfaace Is something wrong with an array to be a React children? I think React children can be an array.

@epicfaace
Copy link
Member

@Kexin-Li that's true, good point -- but in the case shown, items 1 and 2 are null. So in this case, FieldTemplate's children should actually be a single object. It should only be an array when there are multiple items to render.

@ivarprudnikov
Copy link

ivarprudnikov commented Mar 23, 2020

null is a valid child, it can be a placeholder for another element. There is an old interesting discussion over in React about nulls facebook/react#2956

Without seeing how your FieldTemplate's are implemented it is hard to see what caused a problem. But a guess is that single child was expected instead of an array with nulls. There are utility methods in React to deal with similar situations,
eg: React.Children.toArray() https://reactjs.org/docs/react-api.html#reactchildrentoarray

Why we have 2 additional nulls here?

Because after #1118 #1133 2 conditional fields were added:

  • Before:
<FieldTemplate {...fieldProps}>{field}</FieldTemplate>
  • After:
<FieldTemplate {...fieldProps}>
  {field}
  {schema.anyOf && !isSelect(schema) && (<_AnyOfField ... }
  {schema.oneOf && !isSelect(schema) && (<_OneOfField ... }
</FieldTemplate>

To verify open "Any Of" example and you'll see out of those 3 elements only one is null.

@epicfaace
Copy link
Member

@ivarprudnikov I see... do you have a suggestion on how to fix this / are you willing to implement a fix?

@ivarprudnikov
Copy link

ivarprudnikov commented Apr 17, 2020

@epicfaace what I was saying that it is not a bug, it's a normal thing in React and when developer handles elements he almost always needs to make sure it is either an element or an array of elements with nulls as placeholders. If developer wants to get only valid children then they could use toArray:

const myChildren = React.Children.toArray(props.children);

Method interface:

interface ReactChildren {
        // skip other methods for brevity
        toArray(children: ReactNode | ReactNode[]): Array<Exclude<ReactNode, boolean | null | undefined>>;
}

@epicfaace
Copy link
Member

epicfaace commented Apr 21, 2020

@ivarprudnikov I see what you mean, but it might be good to wrap them in a React.Fragment just for ease of use -- what do you think of #1709 as a solution for this?

@ivarprudnikov
Copy link

It depends and in theory could be a breaking change. If anyone was cloning props.children through cloneElement() and doing something with contents then it will break after merge. Because developer will get one fragment instead of field along with optional anyOf, oneOf placeholders. But I do not know if anyone would do that ever.

Also it is not clear what issue creator @mlbstrm was doing with children for this to be an issue at all. Same question could be asked @ketysek as he mentioned that was a problem for his field templates.

In addition to the above if this is something meant only for v2 then PR could be merged. v2 implies there are breaking changes anyway.

@epicfaace
Copy link
Member

fixed by #1709

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants