ApolloProvider's shouldComponentUpdate causes silent issues when used with other providers, such as react router's

[ryancole]

Does ApolloProvider need to implement shouldComponentUpdate?

I just updated my react-apollo version and suddenly my URL routing broke. Because I had only updated react-apollo, I started to track down what was now causing my application to no longer update when the URL changed (using react-router 4 for routing). After a bit of tinkering, I realized that if ApolloProvider was a parent component of react-router's Router then the routing would work, but if Router was the parent of ApolloProvider things broke.

So, I looked at git blame for ApolloProvider's shouldComponentUpdate and see that it was added in a version more recent than the one I had been using. When ApolloProvider is a child of Router the entire routing breaks because ApolloProvider will silently refuse to update itself.

In a typical web application, it's almost guaranteed that there will be several "provider" components and none really specify how they need to be used in conjunction with other providers. I think most providers appear to skip implementing shouldComponentUpdate though because it can cause the whole application to never update.

Does ApolloProvider really need to implement this? If so, I think it should be noted in the docs, if it's not. Ideally somehow there could even be a runtime warning if it detects that you've got a Router child or something, but that's definitely too specific of a warning for this tool.

[helfer]

@ryancole out of curiosity, can you try and see what happens when you remove shouldComponentUpdate? Which react-apollo tests break?

The PR that introduced it seems to have added it mostly out of caution.

[ryancole]

Hmm. I'm trying to run the tests but it says there are no matching tests ...

PS C:\Users\Ryan\Projects\react-apollo> npm test

> [email protected] test C:\Users\Ryan\Projects\react-apollo
> npm run compile && jest


> [email protected] compile C:\Users\Ryan\Projects\react-apollo
> tsc

No tests found
  2258 files checked.
  roots: C:\Users\Ryan\Projects\react-apollo - 2258 matches
  testMatch:  - 2258 matches
  testPathIgnorePatterns: \\node_modules\\ - 65 matches
  testRegex: C:\Users\Ryan\Projects\react-apollo\test\.*.test.(ts|tsx|js)$ - 0 matches
----------|----------|----------|----------|----------|----------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
----------|----------|----------|----------|----------|----------------|
All files |  Unknown |  Unknown |  Unknown |  Unknown |                |
----------|----------|----------|----------|----------|----------------|

> [email protected] posttest C:\Users\Ryan\Projects\react-apollo
> npm run lint


> [email protected] lint C:\Users\Ryan\Projects\react-apollo
> tslint 'src/*.ts*' && tslint 'test/*.ts*'

[jameswyse]

Implementing shouldComponentUpdate should be done cautiously and only when it solves a real performance issue as it has the potential to break a lot of things.

I currently can't upgrade to the latest react-apollo without refactoring a ton of code because of this issue.

[ryancole]

Just wanted to ping back in and mention that I did try running the tests, but I guess I don't know how to run them. As mentioned in my last post, jest says it finds no tests.

[helfer]

@ryancole have tou tried simply running npm test? That's how I run the tests, and it always works for me.

[tizmagik]

FWIW, I pulled latest from master commented out shouldComponentUpdate() in ApolloProvider.tsx and ran the tests and they still all passed. Let me know if you want me to push up a PR with this or how else I can help. Thanks!

[deilv]

The issue was introduced recently in 1614971, perhaps @wmcbain can comment on this, since the commit had no information.

[wmcbain]

Unsure if shouldComponentUpdate is really necessary there. As @helfer said it was introduced mainly out of caution as part of 479. There's other tests in place that check if the new client is being queried against when the client changes so this check is probably extraneous.

[deilv]

My use case has 4 react-routes, one of them uses Apollo to fetch graphql data, which takes about 5sec the first time. After that though every route change takes as long, even for routes that do not use Apollo data at all. My ApolloProvider wraps the router, because wrapping the route only breaks the app. What I mean is that this is a show stopper, and I'm sure it's a very common use case.

[jbaxleyiii]

It's been removed now!