Types are broken with compilerOptions.module set to nodenext due to imports missing file extensions

[jaydenseric]

Intended outcome:

Projects with TypeScript config compilerOptions.module set to nodenext or node16 should be able to import @apollo/client modules with working types. This is critical for using TypeScript correctly with modern Node.js packages or projects that use the package exports field, ESM in .mjs files, etc.

Actual outcome:

Types are broken for @apollo/client modules that have types that import other modules/types without correctly specifying the file extensions in the imports.

How to reproduce the issue:

In package.json:

{
  "name": "demo",
  "private": true,
  "dependencies": {
    "@apollo/client": "^3.6.9"
  }
}

In jsconfig.json:

{
  "compilerOptions": {
    "maxNodeModuleJsDepth": 10,
    "module": "nodenext",
    "noEmit": true,
    "strict": true
  }
}

In demo.mjs:

// @ts-check

import { ApolloLink } from "@apollo/client/link/core/ApolloLink.js";

new ApolloLink((request) => {
  console.log(request);
})

Notice how the types are incomplete for the ApolloLink constructor arguments and fall back to any:

If you dig into the types for it, you can see that the RequestHandler type is being imported incorrectly (missing the file extension) from ./types, and TypeScript is unable to tell what its type is:

If you change the import path to ./types.js (add the missing file extension), the types start to work:

Ultimately fixing the types where ApolloLink is being constructed in the project:

All imports, including for type only imports, should specify the full path including the file name for the module being imported.

As a side note, type only imports should use the type keyword but currently they don't:

apollo-client/src/link/core/ApolloLink.ts

Lines 4 to 10 in 289336a

	import {
	NextLink,
	Operation,
	RequestHandler,
	FetchResult,
	GraphQLRequest
	} from './types';

E.g:

import type {
  NextLink,
  Operation,
  RequestHandler,
  FetchResult,
  GraphQLRequest
} from './types.js';

Versions

• Node.js: v18.4.0
• typescript: v4.7.4
• @apollo/client: v3.6.9

[jaydenseric]

This issue is blocking a big update I'm working on for apollo-upload-client, which (among other things) adds TypeScript types for the package.

[trevor-scheer]

We were able to resolve this same issue for AS4 via apollographql/apollo-server#6731

[GeenCo86]

We were able to resolve this same issue for AS4 via apollographql/apollo-server#6731

What is the result of this. Is there a fix in the client library or will it ?

[trevor-scheer]

@GeenCo86 The outcome of that PR is that this same issue was resolved for Apollo Server in our recent v4 release. Prioritizing and implementing a fix for this in Apollo Client is up to the @apollographql/client-typescript team. I've just provided the PR as additional context.

It's likely they'd be interested in reviewing a PR which provably (i.e. with tests) resolves the issue without breaking existing use cases!

[shaunco]

Any updates on this? This is causing us quite a bit of pain as we try to move from CJS to ESM, and it seems like a pretty simple fix for the Apollo client...

[vtereshyn]

would be great if you guys could provide an update here. thank you in advance!

[jerelmiller]

@phryneas would you mind weighing in here? I'm not sure if this type of change would be considered breaking or whether we can do a minor here.

[phryneas]

I think it might have broader implications, like us also needing to ship .d.cts files etc., but I'm not exactly sure.

I'm gonna set some time aside to experiment with that.

@markerikson, I would be very thankful if you could chime in here.

[markerikson]

What specific change are you considering here?

FWIW, I think this is why I've opted to bundle all of our artifacts for the Redux libs, so I don't have to worry about file extensions inside of transpiled sources (ie, index.ts being built into index.js, and then needing to do export {TheThing} from "./anotherFile.js" internally)

[phryneas]

@markerikson Unfortunately, exactly that one - concatenating all imports with .js and /index.js. We have non-rolled up artifacts, both in types and runtime code. But with my luck, of course that's the one thing you didn't have to fight yet ^^ Thanks for taking a look :)

[vtereshyn]

The problem that we have is here:

import type * as ApolloReactCommon from '@apollo/client';

export type MutationFn = ApolloReactCommon.MutationFunction<Mutation, MutationVariables>;

it worked before when we used CJS, but after refactor to ESM now it says:

Namespace '"@apollo/client/index"' has no exported member 'MutationFunction'.

And this happens with all interfaces, hooks, etc that definitely exported from @apollo/client

To fix that we do

import type * as ApolloReactCommon from '@apollo/client/react/types/types.js';

and

export { useQuery } from '@apollo/client/react/hooks/useQuery.js';
export { useLazyQuery } from '@apollo/client/react/hooks/useLazyQuery.js';
export { useMutation } from '@apollo/client/react/hooks/useMutation.js';

but some of the types that used inside these functions are still broken, but Typescript is not complaining because it happens inside module itself

[jaydenseric]

@markerikson

FWIW, I think this is why I've opted to bundle all of our artifacts for the Redux libs

Never bundle modules that are published, that's a big anti-pattern. It forces users to import and load everything in your library regardless if they are using it or not. Packages with multiple public exports should never have an index module; users should deep import only the export they need from a specific module that has a single export. You can learn more about optimal module design here:

https://jaydenseric.com/blog/optimal-javascript-module-design

[markerikson]

@jaydenseric we've been shipping bundles for most of our libraries for years, and I'm especially doing that now that we're trying to ship full ESM/CJS compat to avoid other pain points.

Tree shaking seems to work fine with Vite/ESBuild, Next/Webpack, etc. Just last night I was converting React-Redux to have bundled artifacts instead of separate transpiled JS files on disk, and confirmed that a Vite app correctly tree-shakes connect from an app bundle if it's not being used.

Note that we assume that almost all of our users are building apps using a bundler, not importing raw ESM into a browser. Along with that, we have embedded process.env.NODE_ENV checks for dev vs prod behavior in our source code. (We are shipping a browser-friendly ESM artifact that has those NODE_ENV checks compiled away in the next majors, specifically to support the "load ESM into a browser" use case.)

(FWIW, I just finished reading your article, and while I appreciate the amount of time you put into writing it, I generally disagree with most of the points being made and don't intend to re-change the packaging setups I've just put months into updating.)

[phryneas]

@jaydenseric @vtereshyn could you give @apollo/client@0.0.0-pr-10994-20230623115940 a spin? (see #10994)

[wootencl]

@phryneas A little late to the party here but super appreciative for tackling this issue 🙏 . In the midst of converting my org's libraries to ESM and this was definitely an unexpected gotcha. Can confirm ~3.8.0 fixes the TS problem on my end and believe this issue can be closed.

/cc @jaydenseric

[phryneas]

Ah right, I forgot to close this after we shipped 3.8 - good call, closing :)

[github-actions]

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.