Circular reference in "bound method"

[iamvery]

Do you want to request a feature or report a bug?

Unclear, but I lean towards bug.

What is the current behavior?

Certain "bound" functions create a circular reference which makes traversing objects problematic.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/84v837e9/).

I'm not sure how to produce a small test case for this issue.

What is the expected behavior?

Unsure.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.0.0-alpha.12
React Native 0.46.4
Jest 20.0.4

This is happening in a test when I attempt to use jsonpath to query the results of renderer.create(<ListItem />).toJSON(). As you can see from that link, I have attempted to address the circular reference in the jsonpath library, but the problem persists. I have managed to track this down to a circular property defined in create-react-class (which is apparently still used by some native components). Here is that snippet of code:

function bindAutoBindMethod(component, method) {
  var boundMethod = method.bind(component);

  boundMethod.__reactBoundContext = component;
  boundMethod.__reactBoundMethod = method;
  boundMethod.__reactBoundArguments = null;

Unfortunately I've yet to narrow that down the precise place this occurs in source as that method definition is repeated in several modules. However I did confirm the issue in this case is isolated to the single file node_modules/create-create-class/factory.js.

The circular reference in particular is the component value in this example. By changing the line from a regular (enumerable) property to one defined with Object.defineProperty (non-enumerable), the issue goes away. So for e.g.

function bindAutoBindMethod(component, method) {
  var boundMethod = method.bind(component);

  Object.define_property(boundMethod, '__reactBoundContext', { value: component });
  boundMethod.__reactBoundMethod = method;
  boundMethod.__reactBoundArguments = null;

That is because the component value contains the circular reference, because all components themselves have a reference to this __reactBoundContext value. So this becomes a problem when traversing the object tree based on its keys as the __reactBoundContext key is enumerated.

You can see my attempt at avoiding circular references here. However unfortunately it still isn't addressing my issue with this particular property. Perhaps my "fix" is naive or otherwise missing a case that I have yet to reproduce in isolation?

Thank you so much for all your hard work and attention to this particularly gritty issue! I must apologize if anything I've said doesn't make clear sense, I am just beginning to get familiar with JS development. ❤️

[gaearon]

Curious: what path leads you to a component instance in the first place? I'd assume JSON output from test renderer doesn't expose them but I might be wrong. (Or maybe you're using a ref or something like this.)

In any case this doesn't sound like a bug to me. There's nothing wrong per se in having circular references. I can't say for sure why your fix is not working, but this doesn't appear to be something we need to fix on our side. In the case you describe the circular reference between a component method and component instance seems expected to me.

[iamvery]

@gaearon Thanks for your reply! I'm not sure I understand your question. I was trying to locate deeply nested values in the results of toJSON() called on a test-rendered component.

I'm certainly not suggesting that the circular reference itself is a bug. However, it might make introspection of these objects a little easier if such properties were not enumerable, read: included in Object.keys.

Regardless, I was able to come up with another solution to my particular case that seems to be working out alright. So feel free to ponder the merits of making such properties non-enumerable, but I won't press for any changes for the time being 😄

Thanks again!

[gaearon]

This code path is very hot. Defining properties is sufficiently slower than a simple assignment that we won't do this there.

[iamvery]

TIL! Here's a reference I found https://www.nczonline.net/blog/2015/11/performance-implication-object-defineproperty/