New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
thebigredgeek opened this Issue Dec 2, 2016 · 17 comments

Comments

Projects
None yet
5 participants
@thebigredgeek
Copy link
Contributor

thebigredgeek commented Dec 2, 2016

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

@thebigredgeek thebigredgeek changed the title Query skip option does not work with redux's connect decorator Query skip option does not work with react-redux's connect decorator Dec 2, 2016

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 2, 2016

Hmmm... this seems very surprising.

If you write

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

what do you see?

@wmertens

This comment has been minimized.

Copy link
Contributor

wmertens commented Dec 2, 2016

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

This comment has been minimized.

Copy link
Contributor Author

thebigredgeek commented Dec 2, 2016

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

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 2, 2016

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

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 5, 2016

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

This comment has been minimized.

Copy link
Contributor Author

thebigredgeek commented Dec 5, 2016

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

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 5, 2016

@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.

@tmeasday tmeasday added the bug label Dec 5, 2016

@tmeasday tmeasday self-assigned this Dec 5, 2016

tmeasday added a commit that referenced this issue Dec 5, 2016

@tmeasday tmeasday added the in progress label Dec 5, 2016

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Dec 5, 2016

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

@thebigredgeek

This comment has been minimized.

Copy link
Contributor Author

thebigredgeek commented Dec 5, 2016

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

This comment has been minimized.

Copy link
Contributor Author

thebigredgeek commented Dec 5, 2016

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 added a commit that referenced this issue Dec 5, 2016

Use instance.fetchData rather than a static method.
Fixes issue with Redux's `connect` hoisting `fetchData` statically.

See #350

tmeasday added a commit that referenced this issue Dec 5, 2016

Use instance.fetchData rather than a static method.
Fixes issue with Redux's `connect` hoisting `fetchData` statically.

See #350

tmeasday added a commit that referenced this issue Dec 5, 2016

Use instance.fetchData rather than a static method.
Fixes issue with Redux's `connect` hoisting `fetchData` statically.

See #350

tmeasday added a commit that referenced this issue Dec 5, 2016

Use instance.fetchData rather than a static method.
Fixes issue with Redux's `connect` hoisting `fetchData` statically.

See #350

jbaxleyiii pushed a commit that referenced this issue Dec 6, 2016

Use instance.fetchData rather than a static method. (#355)
* Use instance.fetchData rather than a static method.

Fixes issue with Redux's `connect` hoisting `fetchData` statically.

See #350

* Added changelog entry
@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 14, 2016

Fixed by v0.7.1

@tmeasday tmeasday closed this Dec 14, 2016

@tmeasday tmeasday removed the in progress label Dec 14, 2016

@thebigredgeek

This comment has been minimized.

Copy link
Contributor Author

thebigredgeek commented Dec 14, 2016

Yay thanks!!!

@cooperka

This comment has been minimized.

Copy link
Contributor

cooperka commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Contributor

tmeasday commented Mar 23, 2018

@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

This comment has been minimized.

Copy link
Contributor

cooperka commented Mar 24, 2018

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

This comment has been minimized.

Copy link
Contributor

wmertens commented Mar 24, 2018

@cooperka

This comment has been minimized.

Copy link
Contributor

cooperka commented Mar 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment