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

Missing "exports" property in package.json #9976

Open dchenk opened 2 years ago

dchenk commented 2 years ago

Intended outcome: Importing '@apollo/client' into an ES module works.

In my experimenting with a fix, I found https://github.com/antfu/vite-ssg/issues/241 and added the postinstall script to add an exports property to the package.json files in the client and client/core packages.

The client package:

"exports": {
    ".": {
      "import": "./index.js",
      "require": "./main.cjs",
      "types": "./index.d.ts"
    },
    "./core": {
      "import": "./index.js",
      "require": "./core.cjs",
      "types": "./index.d.ts"
    },
    "./react": {
      "import": "./react/index.js",
      "require": "./react/react.cjs",
      "types": "./react/index.d.ts"
    },
    "./react/context": {
      "import": "./react/context/index.js",
      "require": "./react/context/context.cjs",
      "types": "./react/context/index.d.ts"
    },
    "./*": "./*"
  }

The client/core package:

  "exports": {
    ".": {
      "require": "./core.cjs",
      "import": "./index.js",
      "types": "./index.d.ts"
    }
  }

Actual outcome: Error from Node.js:

SyntaxError: Named export 'ApolloClient' not found. The requested module '@apollo/client' is a CommonJS module, which may not support all module.exports as named exports.

How to reproduce the issue: Install Node.js version 18.7.0. Set up a project with "type": "module". Install @apollo/client and import it this way:

import { ApolloClient } from '@apollo/client';

Run Node.

Versions

  System:
    OS: macOS 12.4
  Binaries:
    Node: 18.7.0 - ~/.nvm/versions/node/v18.7.0/bin/node
    npm: 8.15.0 - ~/.nvm/versions/node/v18.7.0/bin/npm
  Browsers:
    Chrome: 103.0.5060.134
    Safari: 15.5
  npmPackages:
    @apollo/client: ^3.6.9 => 3.6.9 
websitevirtuoso commented 2 years ago

D we have any changes by this? I have the same problem with 2 more files import { onError } from '@apollo/client/link/error' import { setContext } from '@apollo/client/link/context'

jonkoops commented 1 year ago

I can confirm this also fixes issues with module resolution when using TS Node and ESM.

JasonKleban commented 1 year ago

Is this the right fix or the wrong fix, maintainers? Is it a simple PR?

revmischa commented 1 year ago

Any chance this improves tree-shaking when using esbuild?

benmccann commented 1 year ago

You can see that the package is broken by checking its validity with publint: https://publint.dev/@apollo/client@3.7.14

phryneas commented 1 year ago

@benmccann it is not "broken". That validator has two complaints:

It's an unfortunate situation, but nothing we can immediately act upon without breaking Apollo Client for tons of installations.

jakeisnt commented 1 year ago

Why not re-release and bump the major version? It's not difficult to explain why and there won't then be compatibility issues.

benmccann commented 1 year ago

.cjs.native.js files being CommonJs with a .js file extension. React Native expects files to have a .native.js file extension. These files are only ever read by the React Native Bundler, and that bundler handles these files just fine. No other environment will ever read these files. This is not a problem.

Thanks for pointing this out. I filed an issue with the validator so that hopefully we avoid false reports in the future: https://github.com/bluwy/publint/issues/41

we cannot release a version of Apollo Client 3 with exports.

Agreed that adding exports is a breaking change

this is not "broken"

I think it's correct to say that the issues in the validator are not indicative of something being broken. However, the package is still broken when used directly in a Node.js ESM project without bundling as can be seen in the original issue description, so I'm afraid there is still an issue here besides just following the latest trends and practices unfortunately. Understandable that you'd like to group breaking changes together into a new major release though. Is there anywhere to track when a new major might occur? We've been getting bug reports about this for a couple years (https://github.com/apollographql/apollo-client/issues/8218), so I'm sure users are eager to see a new major

Cellule commented 1 year ago

While I didn't dig much in this particular issue for apollo-client, I suspect the proper fix will have to look like the one made to apollo-server https://github.com/apollographql/apollo-server/pull/7614

Apollo-server was already using exports, so fixing it wasn't a breaking change.

For those on commonjs hitting this issue, I ended up patching (yarn patch or patch-package) all the package.json files of apollo-client to change "type": "module" to "type": "commonjs"

basicdays commented 1 year ago

For what it's worth, I've done the following with patch-package to add a basic exports field to @apollo/client's `package.json:

   "main": "./main.cjs",
   "module": "./index.js",
   "types": "./index.d.ts",
+  "exports": {
+    ".": {
+      "import": "./index.js",
+      "require": {
+        "types": "./index.d.ts",
+        "default": "./main.cjs"
+      }
+    },
+    "./*": {
+      "import": "./*/index.js"
+    }
+  },
   "sideEffects": false,
   "react-native": {
     "./dist/cache/inmemory/fixPolyfills.js": "./cache/inmemory/fixPolyfills.native.js",

The types field is only required if the type definition file is named differently from the imported/required file.

Though this patch won't work if you import submodules of the client in a node.js CJS environment. If the @apollo/client package included their CJS submodule "index" files as index.cjs instead of [submodule_folder_name].cjs, then the patch could be changed to something like the following to support node.js CJS environments:

   "main": "./main.cjs",
   "module": "./index.js",
   "types": "./index.d.ts",
+  "exports": {
+    ".": {
+      "import": "./index.js",
+      "require": {
+        "types": "./index.d.ts",
+        "default": "./main.cjs"
+      }
+    },
+    "./*": {
+      "import": "./*/index.js"
+      "require": {
+        "types": "./*/index.d.ts",
+        "default": "./*/index.cjs"
+      }
+    }
+  },
   "sideEffects": false,
   "react-native": {
     "./dist/cache/inmemory/fixPolyfills.js": "./cache/inmemory/fixPolyfills.native.js",
fernandocanizo commented 1 year ago

I see from the talk that you're talking about React Native, and working on a CJS project, however I'm having this issue in an ESM project (a Remix one) by just following the Get started with Apollo Client tutorial.

I'm new to GraphQL and Apollo Client, and this is plain awful developer experience.

These are my minimal set of steps to reproduce:

npx create-remix@latest # currently v2.1.0
npm install @apollo/client # v3.8.6
npm install graphql # v16.8.1

In package.json: (not a step, Remix already provides this, I'm just mentioning for context)

"type": "module"

Then, after editing app/root.tsx and adding what Apollo Client tutorial says:

import { ApolloClient, InMemoryCache, ApolloProvider, gql } from '@apollo/client';

I get:

SyntaxError: Named export 'ApolloClient' not found. The requested module '@apollo/client' is a CommonJS module, which may not support all module.exports as named exports.
    CommonJS modules can always be imported via the default export, for example using:

    import pkg from '@apollo/client';
    const { ApolloClient, InMemoryCache, ApolloProvider, gql } = pkg;

When searching about this problem I found it happens in several other front-end environments , like Vue and Next.js, so it seems it's affecting a huge base of projects for roughly 3 years now.

Also, there are other imports affected, that documentation says should be done in a way, but then you hit importing problems. For example:

// import { setContext } from "@apollo/client/link/context"; // from tutorial
import { setContext } from '@apollo/client/link/context/index.js'; // how I made it work
// import { loadErrorMessages, loadDevMessages } from "@apollo/client/dev";
import { loadErrorMessages, loadDevMessages } from "@apollo/client/dev/dev.cjs"; // how I made it work

Also the last example gives me: "error| Could not find a declaration file for module '@apollo/client/dev/dev.cjs'" which is an issue I haven't solved yet.

So my question to @phryneas or anyone that could provide a temporary solution, since this won't be fixed until v4.0, how can we circumvent these importing problems? Would it be possible to put some work around here and/or maybe mentioning this stuff in the docs?

Is it possible to fix them on my own code via some black magic in package.json or whatever? I'd really like to not have to tamper with the installed dependencies via patch-package since that would be another step (and possible point of failure) in my project setup.

phryneas commented 1 year ago

@fernandocanizo I hear you - this is a frustrating experience for everyone involved.

The pragmatic resolution in your case is to add a /index.js to every of those imports, which will work without any changes to your bundling and without any package patching.

I've just confirmed that all of

import { ... } from "@apollo/client/index.js";
import { ... } from "@apollo/client/link/context/index.js";
import { ... } from "@apollo/client/dev/index.js";

work fine in Remix.

fernandocanizo commented 1 year ago

Thank you very much indeed @phryneas :D Will replace that .cjs import (probably a copy&paste from who knows where)

phryneas commented 1 year ago

Unfortunately, the bundler of Remix seems to suggest that to you for some reason 😕

andreialecu commented 10 months ago

11570 should fix this

rafagsiqueira commented 7 months ago

@andreialecu I am guessing #11570 was never merged?

laverdet commented 7 months ago

@rafagsiqueira No, the exports change needs to wait for a semver major but Apollo hasn't released a major version in 4 years. My guess is that the team is still approaching major versions as a marketing decision instead of a technical & procedural decision.

Maybe take a look at urql which seems really promising.

jerelmiller commented 7 months ago

@laverdet is right that it will require a major version bump. To correct the record here, we are planning on releasing a major later this year that will include these changes to the exports field. We will be communicating our plans for v4 as soon as we establish our plans for it. We are still in the process of defining what will go into v4 and hope to have something public in the near-ish future. At the very least, I can confidently say this will change will make it into that release.

rafagsiqueira commented 7 months ago

For the time being I ended up patching (yarn patch) apollo-client with the exports suggested on this thread. Waiting for a better solution.

jerelmiller commented 7 months ago

@rafagsiqueira that should be fine! I don't believe we have a better solution in the interim and as long as that works for you, go for it 🙂

rosskevin commented 7 months ago

@jerelmiller @laverdet is correct in assuming that marketing is driving the decision unless you otherwise provide some empirical evidence. No record was corrected with your comment.

Apollo has previously maintained clout as a leader in the graphql space. I am a former committer for react-apollo. To neglect this for so long, in order to apparently maintain a simple matter of not increasing a major version is a disservice to the community.

Reorient to the goal: be the best graphql library.

So far, neglecting progress is in conflict with this goal. Please attempt to move at the speed of the community.

jerelmiller commented 7 months ago

Perhaps "correct the record" was a poor choice of words on my part so I apologize for that. Just trying to communicate here that we are very aware of this issue and plan to release this in a major version which is coming this year. I'm not oblivious to the fact its been 4 years since the last major release. I definitely hear the frustration on that and understand the sentiment.

assuming that marketing is driving the decision unless you otherwise provide some empirical evidence

To be honest, I'm not sure what "empirical evidence" you're looking for here. What I can say is that as core maintainer on the library and not simply an observer, I've been involved in all the planning meetings and discussions thus far on our releases (at least since I've been on the team). The decision not to release a major has nothing to do with a marketing decision but rather our priorities to shipping features in a version that doesn't also require you to spend significant engineering effort to deal with breaking changes to the library.

So far, neglecting progress is in conflict with this goal.

In the last year we've shipped support for React Suspense, been one of the first libraries to implement compatibility with React Server Components, added optimizations to be more efficient with memory, added query preloading for better compatibility with modern routing libraries, and many other features and improvements that are too long to list here. To say that we are neglecting progress is simply false.

Please attempt to move at the speed of the community.

The features listed above have all been community-requested features. To be honest, I'm not entirely sure what you're looking for here. We've got a priority list a mile high and just because we haven't shipped some requested features doesn't mean we aren't shipping them at all or that we are ignoring them.

in order to apparently maintain a simple matter of not increasing a major version is a disservice to the community

Its not simply increasing a major version though. A new major version is a signifier that we've introduced a breaking change in the library. And with any breaking change, this is engineering time and impact to our users to understand what changed, what the upgrade/migration path is, etc. We want to be thoughtful of any breaking change we introduce to ensure we have a good upgrade path. That requires a lot of planning and thought. If we simply YOLO'd changes into new majors, we'd end up with upgrades that require significant engineering effort, which might mean users stuck on old major versions for a significant amount of time.

For this issue in particular, I'll reiterate what Lenz said in the PR:

a change like this would need weeks of rigorous testing, a dedicated release candidate circle etc. etc. - it's not something we can just ship out like that.

Again, we've simply chosen to prioritize shipping new features in the 3.x series of releases where our existing v3 users can enjoy them without having to spend significant engineering effort to upgrade major versions and deal with breaking changes.

Last thing I'll say is that you seem to be on the defensive here. I apologize if my previous comment came across in a negative way because that is certainly not my intent. I'm definitely not looking to pick a fight here, I simply want to defend the work this team has done because I think we've been able to provide a lot of value without having to wait for major versions to do so. Again, I understand the frustration for the long cycle that has been v3 and we are actively working on addressing this.

I hope this helps in some way. Thanks for the response!

rafagsiqueira commented 7 months ago

Just an update for anyone coming to this thread, yarn patch didn't work. I believe it's because apollo-angular imports from it. I am still not sure how to solve this. Perhaps by patching apollo-angular as well, but I will wait for the new apollo-client to come out.

rosskevin commented 7 months ago

@jerelmiller I'm sorry for the tone of my message. I am frustrated.

With every single apollo release, we have to re-yarn patch it in three end products plus multiple internal libraries. That is a frustrating experience and it also adds up to a material "engineering effort" that seems like a waste of time when we feel like an ESM release should have already happened some time ago.

To be honest, I'm not entirely sure what you're looking for here

What I want (and I'm just one user/org):

  1. Maintain the library to the standards e.g. ESM
  2. Add features

In that order. We have an investment in using apollo, as well as many other libraries. There was a critical mass of dependencies using ESM (for us) almost two years ago that required us to switch in order to receive updates to those libraries. Since that time, we have been patching apollo over and over and over again. We have tens (probably more) direct dependencies, which likely extrapolates to a thousand or more in the dependency graph. Apollo is the only dependency we have to patch.

Thank you for the features, but in our case, we would rather have ESM first.

Let me leave off with an apology for my tone, and a sincere thank you for the team's work. Open source can be thankless, and I do not want to appear to be one of those people. I appreciate the efforts, even if we aren't necessarily in alignment with how/when decisions should be taken.

Thank you again.

jerelmiller commented 7 months ago

@rosskevin thanks so much for the response, I sincerely appreciate it. This means more to me than you know.

Massive kudos to you for maintaining the patch for so long. I can certainly understand your frustration having to patch it in multiple places for each and every release (and uh oh we've got another one coming this week 🙈 🏃‍♂️).

I appreciate the efforts, even if we aren't necessarily in alignment with how/when decisions should be taken

Such is the downside to working on a library used by so many developers. Its an incredible honor to do so, but definitely means that its all to easy to make one set of users happy and others frustrated. I suppose thats what keeps this interesting! 😆

Again thanks so much for the response! And thanks so much for using Apollo! It means a lot to us.

jerelmiller commented 7 months ago

@rafagsiqueira have you tried patch-package by chance? I've had pretty good luck with this in the past. Perhaps this might work better for you?

trinode-work commented 4 months ago

Just come across this same issue, We're looking at heavily investing in the apollo ecosystem (router et al), but if marketing (or the desire to add new features) is driving release numbers (which are meant to be defined by what's in them, not adding significant enough features, besides - ESM support is a feature and it's significance is only growing) and delaying things forcing people to monkey patch a critical package for YEARS, there's something wrong.

If adding new features is really forcing a delay to v4 you could easily release a correctly configured sibling package "@apollo/universal-client" and keep the version and code identical to the main client package, after all the difference is only in the package.json, and drop it when v4 comes about. I'm sure the effort will be minimal vs customer satisfaction and the build process could take care of swapping out / mutating the package.json instead of maintaining 2 packages.

steelbrain commented 2 months ago

Documenting here that the solution from @basicdays works very well for our usecase as well

Instead of using patch-package, we're using the latest Yarn (so yarn set version berry) and then using the inbuilt patching mechanism to patch the package (yarn patch @apollo/client). It does suck to have to patch dependencies to work with ESM but it looks like the workaround here will be stable enough that we won't have to revisit it too often