getWrappedInstance returns undefined, when the skip option resolves true

[akomm]

Steps to Reproduce

create:

import {graphql, gql, compose} from 'react-apollo';

const Comp = () => (<div>dummy</div>);
const Composed = compose(
  graphql(gql`query ...`, {withRef: true, skip: true}),
  graphql(gql`query ...`, {withRef: true}),
)(Comp);

use it with ref:

import Composed from './Composed';
import React from 'react';

class App extends React.PureComponent {
  render() {
    return (
      <div>
        <button onClick={() => console.log(this.refs.composedRef.getWrappedInstance())}></button>
        <Composed ref="composedRef" />
      </div>
    );
  }
};

Buggy Behavior

getWrappedInstance returns undefined. When checking with react dev tools, I can see that the first graphql HOC has a wrappedInstance ref, but the second one does not.

Expected Behavior

ref should be propagated down the HOC chain and getWrappedInstance should return the underlying component.

Version

• apollo-client@^1.4.0
• react-apollo@^1.4.0

[akomm]

I think this bug is related to the "skip" option. I glanced into the transpiled code in node_modules and found the following in GraphQL.prototype.render:

GraphQL.prototype.render = function () {
  if (this.shouldSkip()) {
    return createElement(WrappedComponent, this.props);
  }
  var _a = this, shouldRerender = _a.shouldRerender, renderedElement = _a.renderedElement, props = _a.props;
  this.shouldRerender = false;
  if (!shouldRerender && renderedElement && renderedElement.type === WrappedComponent) {
    return renderedElement;
  }
  var data = this.dataForChild();
  var clientProps = this.calculateResultProps(data);
  var mergedPropsAndData = assign({}, props, clientProps);
  if (operationOptions.withRef)
    mergedPropsAndData.ref = 'wrappedInstance';
  this.renderedElement = createElement(WrappedComponent, mergedPropsAndData);
  return this.renderedElement;
};

The wrappedInstance is not merged when this.shouldSkip() returns true.

Yes, it is the bug. Changing like that:

if (this.shouldSkip()) {
    if (operationOptions.withRef) {
      return React.createElement(WrappedComponent, assign({}, this.props, {ref: 'wrappedInstance'}));
    }
    return React.createElement(WrappedComponent, this.props);
}

Fixed the issue, without breaking a whole application running on it.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to React Apollo!

[akomm]

As a workaround:

I don't use ref. I use own prop which is not intercepted by react and does not need HOC forwarding. It accepts a function and passes the reference or null depending on whether the wrapped component mounts or unmounts.

But this is obviously a bug.

Don't understand the logic behind staling untouched/unlabeled issues. Is it desired that those are getting bumped regularly? I don't think so.

[jbaxleyiii]

@akomm would you be able to open a PR with your described change or a failing test?

The stale bot was configured too aggressively, but is useful for projects with a single maintainer (me) to make sure people who request / report something are still around when I get to fixing it :(

I'll make some adjustments to better explainer that / change the label too

[akomm]

Hallo. I understand the problem.

I wanted to add a test a while ago. After first clone I usually check .travis and perform quick "as is" check to see that I start from a "clean" positive test result. Also to see if there are any external dependencies for test which can not be covered by package.json config (like in some projects database/adapter, etc.).

The tests failed (can not recall now what exactly it was). I noticed that you partially use yarn and partially npm to install deps prior to test, but yarn without the --pure-lockfile. My assumption was that the error might be related to different deps versions installed in my environment. Usually I would then run it on travis if it does not work local, but I did not follow it further as I had stuff to get done and I just used the workaround I posted.

[akomm]

Got the test running. Was kind of silly, but that happens when you try to do stuff with limited time. The compile step was important for some tests to run.