Skip to content

Conversation

@alissawix
Copy link
Contributor

@alissawix alissawix commented Aug 20, 2025

fragment children, like any other jsx, could be an array or the single child.
the problem is, that for fragments the fiber.props are props.children (same happens for arrays). and we are using these fiber.props as a key in WeakMaps (props to fiber(s) mapping for example) and it won't work if the single child of a fragment is a primitive (like a string).
** in next PR we will also use this for the __propsToSource map.

we also update the isStatic arg in this case.
this flag actually means "is static children array" so it is false for a single child but we need it to be true when we wrap this single child in an array.

in all other cases we leave it as is.

@alissawix alissawix requested a review from AviVahl August 20, 2025 05:54
Copy link
Contributor

@AviVahl AviVahl left a comment

Choose a reason for hiding this comment

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

while the PR itself is ok, I'd probably write this "handle fragments" section differently now that it has two alterations...

const jsxDEVKeepSource: typeof ReactDevRuntime.jsxDEV = (type, props, key, isStatic, source, self) => {
    if (type === ReactDevRuntime.Fragment) {
        if (source) {
            // we add a key on fragment so it will always have a fiber.
            // with current react version (19) they don't have a fiber if there is no key and they are an only child.
            // to test if it is still needed add a component returning a fragment and see if the fragment has fiber
            key ??= generateKeyFromJSXSource(source);
        }
        if (isObjectLike(props) && 'children' in props && !Array.isArray(props.children)) {
            // if there is a single child, we wrap in an array. so it will always be an object and can be used as key in a WeakMap
            // and tell react that this is a static children array. to avoid warning about a missing key.
            // it seems that keys validation is the only thing that the "isStatic" flag is used for.
            props = { ...props, children: [props.children] };
            isStatic = true;
        }
    }

    const reactElement = ReactDevRuntime.jsxDEV(type, props, key, isStatic, source, self);

    if (source && isObjectLike(reactElement.props)) {
        propsToSource.set(reactElement.props, source);
    }
    return reactElement;
};

benefits:

  • comments in the correct locations
  • no additional variable names, which can cause confusion (isStatic vs isStaticChildrenArray; key vs elementKey).
  • if an adjustment isn't made, the value is kept as-is (same as current flow, but clearer that nothing changes)
  • clearer "we're in fragment realm" scope (instead of two different ternaries)
  • ifs are testing positive comparisons and use &&, making them easier to read
  • one less fn (updateFragmentChildrenToArray) call

@alissawix
Copy link
Contributor Author

since there is another PR on the way
will change the "if"s structure in the next PR

@AviVahl AviVahl merged commit 6dfda6b into main Aug 20, 2025
6 checks passed
@AviVahl AviVahl deleted the alissav/fragment-children-unique-array branch August 20, 2025 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants