Query skip option does not work with react-redux's connect decorator

[thebigredgeek]

Steps to Reproduce

import React, { Component } from 'react';
import { graphql } from 'react-apollo';
import gql from 'graphql-tag';
import { connect } from 'react-redux';

@connect(
  (state) => ({
    isAuthenticated: !!state.app.auth.token
  })
)
@graphql(gql`
  query getMyUser {
    user: getMyUser {
      id
      name
      Customer {
        id
        signupStep
      }
      Expert {
        id
        signupStep
      }
    }
  }
`, {
  skip: ({ isAuthenticated }) => isAuthenticated === undefined || !isAuthenticated
})
class Foo extends Component {
  ...
}

Buggy Behavior

I have to ensure that isAuthenticated isn't undefined before checking if it is false because the skip callback seems to fire BEFORE the connect method. This seems to happen regardless of whether @connect occurs above or below @graphql.

Expected Behavior

ownProps should be decorated by @connect, if used, before skip is called.

Version

• apollo-client@0.4.22
• react-apollo@0.5.13
• react-redux@4.4.5

[tmeasday]

Hmmm... this seems very surprising.

If you write

skip(ownProps) { console.log(ownProps); }

what do you see?

[wmertens]

I have seemingly the same behavior happening when using SSR, during the getDataFromTree phase. With this setup:

@connect(
	({globalSearch: name}) => ({
		name,
		doSearch: Boolean(name),
	}),
	{setFilter}
)
@graphql(SearchResultsQuery)
class MainSearchbox extends Component {

=>

{ Invariant Violation: The operation 'FindClients' wrapping 'MainSearchbox' is expecting a variable: 'doSearch' but it was not found in the props passed to 'Apollo(MainSearchbox)'

When I instrument the @graphql, it turns out that it is not getting the @connect props. I suspect https://github.com/apollostack/react-apollo/blob/master/src/server.ts#L118-L131 calls the queries outside of the @connect-ed component scope.

[thebigredgeek]

Same. This appears to only happen for me during SSR. Wondering if Apollo is somehow bypassing @connect when it fetches data from tree? It seems like Apollo should respect the component tree and default state?

[tmeasday]

Oh! This is during ssr? Ok, I'll look into it

[tmeasday]

So the problem here is that connect hoists fetchData onto the wrapped (Connect(GraphQL(X))) component. So it runs for the connect wrapper with the wrong props etc.

A work around would be to insert some kind of simple wrapper between the two that short-circuits this. For instance, you can probably use one of the things in this library: https://github.com/acdlite/recompose, like

@connect(...)
@defaultProps({foo: 'bar'})
@graphql(...)

@jbaxleyiii I can think of two solutions:

a. In fetchData, somehow detect if we are "hoisted", and do nothing if so. Also we'd probably need to test for this in server.ts. This seems kind of crappy.

b. Not make fetchData a static function at all. I'm not really sure what the case to make it static is anymore, TBH, but there may be a reason that I don't recall. We do instanciate the element before checking anyway so, there's no problem with making it element.fetchData() (rather than Component.fetchData()), which would simplify graphql.tsx a little bit too.

[thebigredgeek]

Couldn't you just recurse down through wrapped components to find the "base"?

[tmeasday]

@thebigredgeek that's more or less what I'm suggesting in a. The problem is that I think one of the major reasons for making it a static property was so other libraries could define their own fetchData. If we are going to detect "one true fetchData's", then it's kind of pointless.

[jbaxleyiii]

@tmeasday I think we should remove the static nature of fetchData! 👍

[thebigredgeek]

One interesting albeit unorthodox approach might be to append a uuid suffix to the fetchData namespace that is computed once at startup and used everywhere in constant form. This would prevent collision. You could then expose a "public" version with the name simply "fetchData". This way if it is overridden apollo still works

[thebigredgeek]

I agree though. fetchData should be hidden IMO. Perhaps some sort of public interface could be exposed, but I think for safety it makes more sense for the public interface to be a reference to a private interface that is guaranteed to work

[tmeasday]

Fixed by v0.7.1

[thebigredgeek]

Yay thanks!!!

[cooperka]

Hi @tmeasday, it looks like this is still an issue as of react-apollo v2.0.4 when the connect decorator is executed before the graphql decorator. This seems like a bug, since the props should be available in any case, especially after the component has already been connected to Redux.

See this StackOverflow question for more context: How To Pass Props From Redux Store into Apollo GraphQL Query?

Luckily the workaround is effective for some users, but I like to test my components separately from Apollo -- I use several decorators to give the test component its static props, but mock the Apollo data -- thus graphql must come after connect, thus the Redux props aren't available in skip.

Do you think this is solvable?

---

Note: Not sure how I "unassigned" you? That was automatic I think.

[tmeasday]

@cooperka I don't think it can really conceptually work to do it the other way, if you think about how the component heirarchy is working in this case.

If you want to test the component without graphql, you will need to test it without connect too. If you are using connect to supply props to graphql, I don't think this would be an issue anyway?

If you are using connect for other component props, you might consider using connect twice?

export default compose(
  connect(mapPropsForGraphql),
  graphql(...),
  connect(mapPropsForComponent))(Component)

Does that help?

Note: Not sure how I "unassigned" you? That was automatic I think.

Ha! you have special powers.

[cooperka]

Hmm, that does make sense -- I was originally thinking about props as being defined on the component, but really they're being passed into the component, in which case connect passes the props into the graphql HOC.

I was thinking graphql could statically analyze whatever data connect has added to the component in order to grab the props, but clearly that would be overly complex and out of the scope of Apollo's job.

I solved my issue by simply mocking the graphql HOC with Jest:

// __mocks__/react-apollo.js

module.exports = {
  ApolloProvider: ({ children }) => children,
  graphql: () => (component) => component,
};

Thanks again for the help!

[wmertens]

The new component-based API in 2.1 sidesteps this whole issue, you can do the proper composing yourself. Nice :)

…

On Sat, Mar 24, 2018 at 5:29 PM Kevin Cooper ***@***.***> wrote: Hmm, that does make sense -- I was originally thinking about props as being defined on the component, but really they're being passed *into* the component, in which case connect passes the props *into* the graphql HOC. I was thinking graphql could statically analyze whatever data connect has added to the component in order to grab the props, but clearly that would be overly complex and out of the scope of Apollo's job. I solved my issue by simply mocking the graphql HOC with Jest: // __mocks__/react-apollo.js module.exports = { ApolloProvider: ({ children }) => children, graphql: () => (component) => component, }; Thanks again for the help! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#350 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlrf8VDXDAavcnltG4LcExng1wLZaks5thnRZgaJpZM4LCFbF> .

[cooperka]

@wmertens that's excellent -- I just read the blog post. Nice!