Do not clone key and ref getter props

[ericmatthys]

When passing all props to cloneElement in a dev environment, the key and ref getters that are automatically set on props trigger warnings unexpectedly (e.g. key is not a prop. Trying to access it will result...). This changes ReactElement to use propertyIsEnumerable to decide whether to use the key and ref from the config object, so the getters are ignored.

[jimfb]

Flagging for performance: This adds two call to propertyIsEnumerable in the production code path. We may want to just disable the warning when cloning, similar to what I did in https://github.com/facebook/react/pull/5976/files

@ericmatthys Let's make sure we add a unit test to verify the exact case you're talking about. Write a simple component that passes the props object into cloneElement and assert that it does not warn.

On a higher level note, I wonder how we feel about passing the props object directly to cloneElement as a config. It means the cloned child will receive all the props of the parent, when the owner should probably be passing those props directly to the child. @ericmatthys can you elaborate (in detail) why you are using this pattern?

[ericmatthys]

Thanks @jimfb.

An example of a possibly problematic component:

function Outer(props) {
  const {
    children,
    populated,
    error
  } = props;

  if (populated) {
    const child = React.Children.only(children);
    return React.cloneElement(child, props);
  }

  if (error) {
    return <Error />;
  }

  return <Spinner />;
}

Let's assume there's a "connector" component that provides props, like populated and error, to Outer that it selects from app state. In that component's render function, it'd return Outer and provide it the props that were selected from a store:

// OuterConnector

render() {
  return <Outer {...this.props} />;
}

<OuterConnector store={store}>
  <InnerFoo />
</OuterConnector>

<OuterConnector store={store}>
  <InnerBar />
</OuterConnector>

This lets OuterConnector / Outer wrap different components, passing them props that would otherwise not be available wherever <OuterConnector> is used. This is useful when composing multiple "connectors" that provide different sets of data.

<OuterConnector1 store={store}>
  <OuterConnector2>
    <InnerFoo />
  </OuterConnector2>
</OuterConnector1>

A workaround to the issue, which does not have very obvious intentions:

  const {
    children,
    populated,
    error,
    ...otherProps
  } = props;

  if (populated) {
    const child = React.Children.only(children);
    return React.cloneElement(child, {
      populated,
      error,
      ...otherProps
    });
  }

If you think that approach is dangerous or broken, it'd be good to have a warning that is relevant to the violating use case. Otherwise, I can add better tests.

[gaearon]

I suggest you to extract something like

function getRef(config) {
  if (__DEV__) {
    return !config.hasOwnProperty('ref') ||
      Object.getOwnPropertyDescriptor(config, 'ref').get ? null : config.ref;
  } else {
    return config.ref === undefined ? null : config.ref;
  }
}

function getKey(config) {
  if (__DEV__) {
    return !config.hasOwnProperty('key') ||
      Object.getOwnPropertyDescriptor(config, 'key').get ? null : '' + config.key;
  } else {
    return config.key === undefined ? null : '' + config.key;
  }
}

Then you can use them both in createElement() and cloneElement(). Another nice thing about this: it avoids '' + config.key duplication as well.

Would you like to give it a try?

[ericmatthys]

@gaearon Yes, I will update with an approach similar to that.

[ericmatthys]

@gaearon Sorry for the delay. It's ready for another look now.

[facebook-github-bot]

@ericmatthys updated the pull request.

[gaearon]

This doesn't merge cleanly, can you look into why? Thanks.

[ericmatthys]

A warning was added to enforce props is a plain object. It should merge cleanly now.

[facebook-github-bot]

@ericmatthys updated the pull request.

[gaearon]

Any specific reason why you're changing comparisons to be looser here? They used to be true for null but are now false.

[ericmatthys]

The configRef value is returned by getRef and can be null. We could strictly check null, but an undefined or null check felt safer.

[ericmatthys]

If null values should be allowed here, I think we'd need to check config.ref and config.key before calling getRef and getKey and then always set ref and key to the return.

[ericmatthys]

Allowing key to be null seems like a mistake, since it is coerced to the string "null".

[ericmatthys]

I think this approach should keep behavior consistent. Let me know what you think and I'll add a test for the null cases.

function isValidConfigRefOrKey(config, name) {
  if (__DEV__) {
    return config.hasOwnProperty(name) &&
      !Object.getOwnPropertyDescriptor(config, name).get;
  }

  return config[name] !== undefined;
}

function getConfigKey(config) {
  return '' + config.key;
}

...

ReactElement.createElement = function(type, config, children) {
  ...

    if (isValidConfigRefOrKey(config, 'ref')) {
      ref = config.ref;
    }

    if (isValidConfigRefOrKey(config, 'key')) {
      key = getConfigKey(config);
    }

...

ReactElement.cloneElement = function(element, config, children) {
  ...

    if (isValidConfigRefOrKey(config, 'ref')) {
      // Silently steal the ref from the parent.
      ref = config.ref;
      owner = ReactCurrentOwner.current;
    }

    if (isValidConfigRefOrKey(config, 'key')) {
      key = getConfigKey(config);
    }

[gaearon]

This look sensible to me. More tests would be welcome 👍

[facebook-github-bot]

@ericmatthys updated the pull request.

[jimfb]

@ericmatthys Ok, I see your use case, you are wrapping Connectors and want all the props to propagate down. I think you are running into this issue because you are using connectors to pull data out of your application state, rather than pulling out the application info at the application-component level.

I think a better way to structure your application would be:

<OuterConnector1 state={store.getWhateverState()}>
  <OuterConnector2 error={store.hasError()} populated={store.isPopulated()}>
    <InnerFoo bar={store.getMyVariable()} noise={store.getMyOtherVariable()} />
  </OuterConnector2>
</OuterConnector1>

It is a little more typing, but it's far more explicit. Your current method of having connectors pull data out of and do a bucket-brigade of blindly passing/forwarding props makes the code much harder to reason about because you don't know what props are expected at any given stage (ie. it runs into a lot of the same maintainability problems as context).

[ericmatthys]

@jimfb I definitely agree, and I'm in the process of refactoring my use of connectors and stores to support that. For now, I think I'm stuck with the more implicit method due to how connectors are working in my case.

Either way, it seems good to handle ref and key from config the same when creating and cloning elements, or be very strict about it and warn.

[gaearon]

Can this indirection (passing 'ref' instead of directly reading config.ref) cause any performance regressions in the production path? I’m not well versed in optimizations like this but it’s a super hot code path so we better make sure we don’t regress on it in prod.

[ericmatthys]

We're adding potentially 3 function calls. It could very easily be reduced to 2 since getConfigKey isn't very necessary. The function has the same undefined condition that existed previously, but it now skips assigning null again since the value is already null. That doesn't seem like enough to make any measurable difference in performance, but I'm sure if you have any thoughts on proving that empirically.

[max-ch9i]

@jimfb

(ie. it runs into a lot of the same maintainability problems as context)

This is why I use cloneElement with this.props. I want the child component to know all properties of the parent, providing the context.

My use case is:

let children = React.Children.map(this.props.children, child => React.cloneElement(child, this.props));

[dlong500]

Any updates on this or why this hasn't been merged? This issue rears its head on any use of cloneElement as far as I can tell.

[gaearon]

Thanks a lot for the PR, and sorry it took a long time to review! I started merging this, but then found a few existing issues in the same area, and so I rebased your commit and added a bunch of new ones on top. Your PR helped surface those bugs!

I’m closing but your commit will get in as a part of #6880.