apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.4k stars 2.66k forks source link

Undocumented backwards incompatibility between v2 and v3 -- @apollo/client crashes trying to render ApolloProvider by importing its own react/index.js file when resolving source dir as modules with webpack #7464

Closed HeyParkerJ closed 11 months ago

HeyParkerJ commented 3 years ago

Summary: @apollo/client will crash by attempting to import its own react/index.js file instead of node_modules/react if your webpack config is set to resolve your own source code as modules.

Intended outcome: I follow the upgrade guide and am successfully able to use Apollo v3.

Actual outcome: I follow the upgrade guide and get runtime errors and build warnings:

ApolloContext.ts:23 Uncaught TypeError: Cannot read property 'createContext' of undefined
    at getApolloContext (ApolloContext.ts:23)
    at ApolloProvider (ApolloProvider.tsx:16)
    at renderWithHooks (react-dom.development.js:14985)
    at mountIndeterminateComponent (react-dom.development.js:17811)
    at beginWork (react-dom.development.js:19049)
    at HTMLUnknownElement.callCallback (react-dom.development.js:3945)
    at Object.invokeGuardedCallbackDev (react-dom.development.js:3994)
    at invokeGuardedCallback (react-dom.development.js:4056)
    at beginWork$1 (react-dom.development.js:23964)
    at performUnitOfWork (react-dom.development.js:22776)
WARNING in ./node_modules/@apollo/client/react/context/ApolloConsumer.js 6:11-30
export 'default' (imported as 'React') was not found in 'react' (possible exports unknown)
 @ ./node_modules/@apollo/client/react/context/index.js 1:0-36 1:0-36
 @ ./node_modules/@apollo/client/react/index.js 1:0-106 1:0-106 1:0-106 1:0-106 1:0-106
 @ ./node_modules/@apollo/client/index.js 2:0-33 2:0-33
 @ ./src/App.tsx 7:15-40
 @ ./src/index.tsx 8:28-44

How to reproduce the issue: This is not a new issue, @Billy- was able to create a small reproduction repo in #7370 that I was able to fork to dig further.

https://github.com/HeyParkerJ/apollo-client-test

In this repo there are 3 branches. Do note that I upgraded @apollo/client version to 3.3.5 to remove the sourcemap warnings originally brought up in @Billy-'s issue.

  1. master - Broken. This is set up using Apollo v3 instructions and closely mirrors Billy-'s reproduction
  2. fixed - Fixed. This is branched off of Master and implements a single fix -- removing the resolve modules option in webpack.config.js
  3. apolloV2 - Working. This demonstrates an Apollo setup akin to the v2 instructions (though they are incomplete and inaccurate as noted in #3560). The aforementioned webpack configuration mirrors that of the broken Master branch, and yet the app builds and runs just fine. This seems to be a breaking change that does not appear in the migration documents.

I have discovered that the specific issue is when I resolve my own modules within webpack using

resolve: {
  modules: ['node_modules', '.'], // '.' can be replaced with client, app, etc
}

While removing the resolve configuration works for this minimal reproduction, this requires significant overhaul for the original repo that I discovered this issue on. I have not yet dug into what configuration has changed with @apollo/client to cause this, but I do want to note this breaking change that is not noted in the docs. It is worth noting that this type of configuration does mirror the webpack examples.

Versions master branch: System: OS: macOS Mojave 10.14.6 Binaries: Node: 14.5.0 - ~/.nvm/versions/node/v14.5.0/bin/node Yarn: 1.21.1 - /usr/local/bin/yarn npm: 6.14.5 - ~/.nvm/versions/node/v14.5.0/bin/npm Browsers: Chrome: 87.0.4280.88 Firefox: 78.5.0 Safari: 14.0.1 npmPackages: @apollo/client: 3.3.5 => 3.3.5

benjamn commented 3 years ago

@HeyParkerJ Thanks for digging into this. Can you exclude node_modules/@apollo/client (or even all of node_modules) from that webpack configuration? Resolving source code directories as packages is a surprise no library should have to tolerate, especially because the contents of node_modules are not "your own source code."

HeyParkerJ commented 3 years ago

@benjamn If I'm understanding correctly, that seems quite unstandard for a webpack config. If I remove node_modules from the resolve config (and left the resolve: ['.']), I start getting issues like:

Module not found: Error: Can't resolve 'react' in '/Users/parker.johnson/Documents/git/apollo-client-test/src'

Removing node_modules from the config would mean I could no longer import React from 'react';. Rather, I would have to import React from '../node_modules/react. I configured an example branch here. Excluding node_modules/@apollo/client would have similar effects on being able to import it without relative file path declarations.

That being said, if you remove the resolve entry entirely, webpack seems to fall back to sensible defaults which allows the app to run (though the consumer needs to switch their source code imports to complete filenames+extension as well as direct location ex: import App from './App.tsx. This lead me to begin thinking that the '.' portion of the resolve is funky.

Since messing around with your suggestion a bit, I'm noticing that if I take the minimal reproduction code and change the config from this:

resolve: {
    extensions: ['.ts', '.tsx', '.js', '.jsx', '.json'],
    modules: ['node_modules', '.'],
  },

to this:

resolve: {
   extensions: ['.ts', '.tsx', '.js', '.jsx', '.json'],
   modules: ['node_modules', path.resolve(__dirname, 'src')],
},

Then the minimal reproduction repo begins working. So it appears that the ['.'] actually is faulty.

To explain my original conclusion, my original repo looks like this:

  resolve: {
    extensions: ['.css', '.mjs', '.js', '.jsx'],
    modules: ['node_modules', 'client'],
  },

And adding a similar path.resolve(...) fix to that repo does not fix the issue, so while I still believe there is some level of breaking change that isn't noted in the migration guide, I am able to mitigate it by no longer adding my source code folder to the webpack resolve configuration. Ultimately I am unable to reproduce this issue in the minimal reproduction code if I switch to the path.resolve() method.

For what it's worth, this user also had this issue, so I think there's more to be discovered here even though I am able to work around it in my main repo and unable to reproduce in my example repo.

https://spectrum.chat/apollo/react-apollo/error-when-integrating-with-existing-project~9ff0b660-6ed5-41a4-9e02-307019cc8e67

benjamn commented 3 years ago

So it appears that the ['.'] actually is faulty.

Yes, I was suggesting you remove the '.' from the modules: ['node_modules', '.'] array, not the 'node_modules' string. Sorry for not being clearer.

If the '.' applied only to application source code, it would probably be fine, but it seems like it's also applying to library code inside node_modules, because it modifies the resolution rules that

node_modules/@apollo/client/react/context/ApolloConsumer.js

uses to resolve the react package, mistakenly resolving node_modules/@apollo/client/react/index.js. That should never happen.

To put this another way, to fix this problem in Apollo Client, we would have to change something about imports like

import React from "react"

… and there's just nothing about that import declaration that could possibly be wrong (other than possibly needing to use * as React instead of React, though I think that's irrelevant here). If your bundler has trouble with an import like that, then I think it's safe to say you have a bundler configuration problem. I wish there was something Apollo Client could do to help, but I don't think there is.

HeyParkerJ commented 3 years ago

Hey @benjamn, just getting back to this.

So I understand that neither of us fully understand what is causing this backwards incompatibility. I would investigate further, but I would need to learn Rollup to investigate my next theory, which I have not time for at the moment. That being said, would you be willing to agree that my minimal reproduction is sufficient to acknowledge that there is indeed a backwards incompatibility at play here? Anyone else upgrading from v2 to v3 could have a lot of their time and pain saved with a simple note in the migration documentation detailing that an application cannot resolve its own code as modules with webpack in Apollo v3.

Jaikant commented 3 years ago

I am facing the same issue, my setup is nextron + apollo client.

Edit: The issue was resolved by removing the . from resolve.modules.

I tried both available solutions

1) I modified the webpack config resolves to only have the node_modules.

2) The solution here.

But the error persists!

Uncaught TypeError: Cannot read property 'createContext' of undefined
    at getApolloContext (webpack-internal:///../node_modules/@apollo/client/react/context/ApolloContext.js:10)
    at ApolloProvider (webpack-internal:///../node_modules/@apollo/client/react/context/ApolloProvider.js:11)
    at renderWithHooks (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:14985)
    at mountIndeterminateComponent (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:17811)
    at beginWork (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:19049)
    at HTMLUnknownElement.callCallback (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3945)
    at Object.invokeGuardedCallbackDev (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:3994)
    at invokeGuardedCallback (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:4056)
    at beginWork$1 (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:23959)
    at performUnitOfWork (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:22771)
    at workLoopSync (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:22702)
    at renderRootSync (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:22665)
    at performSyncWorkOnRoot (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:22288)
    at scheduleUpdateOnFiber (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:21876)
    at updateContainer (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:25477)
    at eval (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:26016)
    at unbatchedUpdates (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:22426)
    at legacyRenderSubtreeIntoContainer (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:26015)
    at Object.hydrate (webpack-internal:///../node_modules/react-dom/cjs/react-dom.development.js:26081)
    at renderReactElement (webpack-internal:///../node_modules/next/dist/client/index.js:586)
    at doRender (webpack-internal:///../node_modules/next/dist/client/index.js:761)
    at _callee2$ (webpack-internal:///../node_modules/next/dist/client/index.js:477)
    at tryCatch (webpack-internal:///../node_modules/regenerator-runtime/runtime.js:63)
    at Generator.invoke [as _invoke] (webpack-internal:///../node_modules/regenerator-runtime/runtime.js:293)
    at Generator.eval [as next] (webpack-internal:///../node_modules/regenerator-runtime/runtime.js:118)
    at asyncGeneratorStep (webpack-internal:///../node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:3)
    at _next (webpack-internal:///../node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:25)
    at eval (webpack-internal:///../node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:32)
    at new Promise (<anonymous>)
    at eval (webpack-internal:///../node_modules/next/node_modules/@babel/runtime/helpers/asyncToGenerator.js:21)
    at _render (webpack-internal:///../node_modules/next/dist/client/index.js:514)
    at render (webpack-internal:///../node_modules/next/dist/client/index.js:451)
    at eval (webpack-internal:///../node_modules/next/dist/client/next-dev.js:94)
    at eval (webpack-internal:///../node_modules/next/dist/client/dev/fouc.js:16)
Screenshot 2021-02-17 at 5 31 53 PM
jcpsimmons commented 3 years ago

Thasks @HeyParkerJ for documenting this - been searching for an answer and your comments led me here. Do you know of any other workarounds other than removing '.' from Jest's config? It definitely resolves the issue but the rest of my project is relying heavily on import with a baseURL of .

HeyParkerJ commented 3 years ago

@jcpsimmons Unfortunately, none that I know of. Our team ended up shifting all our imports around to support the new import structure.

Edit: On second thought, the edit in this Spectrum post below mentioned that he was able to alias apollo-client to use a precompiled version. I was unable to get this to work, but if you're able to, please let us know!

https://spectrum.chat/apollo/react-apollo/error-when-integrating-with-existing-project~9ff0b660-6ed5-41a4-9e02-307019cc8e67

"EDIT This is in someway related to webpack loader and current project configuration, my project is detached and use a 2 years old webpack file with a lot of changes. I added an alias for @apollo /client on webpack.config.js to force it to use its compiled version and it worked:"

resolve: { alias: { '@apollo/client$': 'node_modules/@apollo/client/apollo-client.cjs.min.js', }, },

hwillson commented 3 years ago

It doesn't sound like there is an outstanding issue here, so closing. Thanks!

HeyParkerJ commented 3 years ago

@hwillson I have written quite a few words here to detail why I believe the opposite is true, including taking time to create reproduction. My understanding is that this was left at the juncture where I am able to prove a backwards incompatibility and/or limitation of Apollo v3, but am unable to continue digging as to what change introduced this as I am not as intimately familiar with the code as a maintainer. In light of not being granted investigation as to why this limitation is the case, I proposed that the limitation simply be documented to save others the time I spent on the issue, but that has not been replied to.

I don't think that this has been resolved or addressed, so I'm curious as to what gave the impression that there was not an outstanding issue here?

hwillson commented 3 years ago

Thanks @HeyParkerJ - I was basing the closing of the issue off of the comments in https://github.com/apollographql/apollo-client/issues/7464#issuecomment-743473623.

I proposed that the limitation simply be documented to save others the time I spent on the issue, but that has not been replied to.

I'll re-open to track this. 👍

Billy- commented 3 years ago

I managed to find a fix for this that still works with the module alias as far as I can tell - given that my webpack context was set to ./src I could set my module alias to ./src instead of . and is resolves the issue and everything still works as expected. Example here: https://github.com/Billy-/apollo-client-test/commit/6b899187f6e4c47708f8caa452bd2d4fbf14195b (I have used the absolute path as it's a re-used variable, and this works, but using 'src' or './src' works too, just not '.').

What I don't get is that . would normally set my module alias to start from the webpack context (./src), so I'd expect setting it to ./src would send it looking for ./src/src. But for some reason it works and my aliases resolve from ./src as desired.. 🤷‍♂️

Unless I am misunderstanding something, this does still seem like strange behaviour, and if we can't track down and fix the cause, I agree that it would probably be good to at least document this quirk and the workaround (assuming this works for others too)

paigekim29 commented 3 years ago

@jcpsimmons Hello, I just wonder whether you find a way of keeping '.' on your jest.config.js because my project is relying on import with a baseURL of '.' as yours.

phryneas commented 11 months ago

Apollo Client 3.9 will not directly import from the react package anymore, so this problem should be solved for you the moment it is released (we are in beta right now - please give it a spin :) ).

github-actions[bot] commented 10 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. For general questions, we recommend using StackOverflow or our discord server.