Skip to content
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

Add "type": "module" to @apollo/client/core and friends #8396

Merged
merged 4 commits into from
Jul 28, 2021

Conversation

bennypowers
Copy link
Contributor

Consider trying to load @apollo/client/core in node.js

node -v # 16.3.0
// server/index.js
import { ApolloClient } from '@apollo/client/core';

import { ApolloClient, InMemoryCache } from '@apollo/client/core/index.js';
^^^^^^^^^^^^
SyntaxError: Named export 'ApolloClient' not found. The requested module '@apollo/client/core/index.js' 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 AC from '@apollo/client/core';
const { ApolloClient } = AC;

(node:35704) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use node --trace-warnings ... to show where the warning was created)
/path/to/node_modules/@apollo/client/core/index.js:1
export { ApolloClient, mergeOptions, } from "./ApolloClient.js";
^^^^^^

SyntaxError: Unexpected token 'export'

adding "type": "module" to core will solve those errors nicely

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@bennypowers bennypowers changed the title Add "type": "module" to @apollo/client/core Add "type": "module" to @apollo/client/core and friends Jun 18, 2021
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this started @bennypowers! I think I have an idea for how to make this work. I'll push another commit or two to show you what I mean.

Comment on lines 71 to 74
main: `${bundleName}.cjs.js`,
module: 'index.js',
types: 'index.d.ts',
type: "module",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand type: "module" correctly, it directs Node to treat all .js files within the package (including the main entry point) as ECMAScript modules, rather than CommonJS modules. However, the bundle that we're putting in the main field above (note the cjs in ${bundleName}.cjs.js) definitely still is CommonJS, not ESM, which is why this one-line change isn't quite enough, by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah for cjs you can give it a .cjs extension.

Adopting export maps is a more comprehensive solve

Copy link

@Venryx Venryx Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, someone raised an issue for this behavior of NodeJS here: nodejs/node#34515

Basically, the complaint is that NodeJS should be automatically detecting index.js as an ESM file based on the module: index.js entry in package.json. (same also for the exports.import field -- it's currently ignoring even that as a "hint")

I agree with the OP of that issue -- though the NodeJS team hasn't really followed up on the complaint/suggestion so far (which is understandable due to how many active issues they have to deal with).

@benjamn benjamn changed the base branch from main to release-3.4 June 23, 2021 14:49
@bennypowers
Copy link
Contributor Author

Thanks @benjamn for checking into this. Is adopting export maps of interest?

@alexanderniebuhr
Copy link

@benjamn benjamn deleted the branch apollographql:release-3.5 July 28, 2021 17:12
@benjamn benjamn closed this Jul 28, 2021
@benjamn
Copy link
Member

benjamn commented Jul 28, 2021

This should not have been closed, sorry! Merging release-3.4 into main led to GitHub deleting the release-3.4 branch, which closed PRs targeting that branch.

@benjamn benjamn reopened this Jul 28, 2021
bennypowers and others added 3 commits July 28, 2021 13:51
Consider trying to load `@apollo/client/core` in node.js

```bash
node -v # 16.3.0
```
```js
// server/index.js
import { ApolloClient } from '@apollo/client/core';
```
> import { ApolloClient, InMemoryCache } from '@apollo/client/core/index.js';
>          ^^^^^^^^^^^^
> SyntaxError: Named export 'ApolloClient' not found. The requested module '@apollo/client/core/index.js' 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:

```js
import AC from '@apollo/client/core';
const { ApolloClient } = AC;
```
> (node:35704) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
> (Use `node --trace-warnings ...` to show where the warning was created)
> /path/to/node_modules/@apollo/client/core/index.js:1
> export { ApolloClient, mergeOptions, } from "./ApolloClient.js";
> ^^^^^^
>
> SyntaxError: Unexpected token 'export'

adding `"type": "module"` to core will solve those errors nicely
For example, this rewrites the import from "ts-invariant/process" in
@apollo/client/utilities/globals/graphql.js to instead import from
"ts-invariant/process/index.js, so Node.js won't complain about the
directory import (even though ts-invariant/process/package.json exists).
@benjamn benjamn changed the base branch from release-3.4 to release-3.5 July 28, 2021 17:52
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this into the next/first v3.5 beta. Thanks for waiting @bennypowers!

@benjamn benjamn merged commit 57c451f into apollographql:release-3.5 Jul 28, 2021
@bennypowers
Copy link
Contributor Author

bennypowers commented Jul 28, 2021

Awesome

I got an SSR setup running with this patch

Click to see Patch
diff --git a/node_modules/@apollo/client/cache/package.json b/node_modules/@apollo/client/cache/package.json
deleted file mode 100644
index a0dd9da..0000000
--- a/node_modules/@apollo/client/cache/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/cache",
-  "main": "cache.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/core/package.json b/node_modules/@apollo/client/core/package.json
deleted file mode 100644
index 3d86a01..0000000
--- a/node_modules/@apollo/client/core/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/core",
-  "main": "core.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/errors/package.json b/node_modules/@apollo/client/errors/package.json
deleted file mode 100644
index 6019e5d..0000000
--- a/node_modules/@apollo/client/errors/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/errors",
-  "main": "errors.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/batch-http/package.json b/node_modules/@apollo/client/link/batch-http/package.json
deleted file mode 100644
index 49d229a..0000000
--- a/node_modules/@apollo/client/link/batch-http/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/batch-http",
-  "main": "batch-http.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/batch/package.json b/node_modules/@apollo/client/link/batch/package.json
deleted file mode 100644
index 184f89f..0000000
--- a/node_modules/@apollo/client/link/batch/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/batch",
-  "main": "batch.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/context/package.json b/node_modules/@apollo/client/link/context/package.json
deleted file mode 100644
index 04d7a03..0000000
--- a/node_modules/@apollo/client/link/context/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/context",
-  "main": "context.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/core/package.json b/node_modules/@apollo/client/link/core/package.json
deleted file mode 100644
index c6098e9..0000000
--- a/node_modules/@apollo/client/link/core/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/core",
-  "main": "core.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/error/package.json b/node_modules/@apollo/client/link/error/package.json
deleted file mode 100644
index 5e8b360..0000000
--- a/node_modules/@apollo/client/link/error/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/error",
-  "main": "error.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/http/package.json b/node_modules/@apollo/client/link/http/package.json
deleted file mode 100644
index 54cbcae..0000000
--- a/node_modules/@apollo/client/link/http/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/http",
-  "main": "http.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/persisted-queries/package.json b/node_modules/@apollo/client/link/persisted-queries/package.json
deleted file mode 100644
index 4f776d5..0000000
--- a/node_modules/@apollo/client/link/persisted-queries/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/persisted-queries",
-  "main": "persisted-queries.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/retry/package.json b/node_modules/@apollo/client/link/retry/package.json
deleted file mode 100644
index 3f8bcd2..0000000
--- a/node_modules/@apollo/client/link/retry/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/retry",
-  "main": "retry.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/schema/package.json b/node_modules/@apollo/client/link/schema/package.json
deleted file mode 100644
index ba93f89..0000000
--- a/node_modules/@apollo/client/link/schema/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/schema",
-  "main": "schema.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/utils/package.json b/node_modules/@apollo/client/link/utils/package.json
deleted file mode 100644
index 743cab3..0000000
--- a/node_modules/@apollo/client/link/utils/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/utils",
-  "main": "utils.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/link/ws/package.json b/node_modules/@apollo/client/link/ws/package.json
deleted file mode 100644
index b8be5f3..0000000
--- a/node_modules/@apollo/client/link/ws/package.json
+++ /dev/null
@@ -1,7 +0,0 @@
-{
-  "name": "@apollo/client/link/ws",
-  "main": "ws.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": false
-}
diff --git a/node_modules/@apollo/client/package.json b/node_modules/@apollo/client/package.json
index 6c7898a..0c0dbf2 100644
--- a/node_modules/@apollo/client/package.json
+++ b/node_modules/@apollo/client/package.json
@@ -17,6 +17,20 @@
   "module": "./index.js",
   "types": "./index.d.ts",
   "sideEffects": false,
+  "type": "module",
+  "exports": {
+    ".": "./index.js",
+    "./cache": "./cache/index.js",
+    "./core": "./core/index.js",
+    "./core/*": "./core/*.js",
+    "./link": "./link/index.js",
+    "./link/schema": "./link/schema/index.js",
+    "./link/http": "./link/http/index.js",
+    "./link/utils": "./link/utils/index.js",
+    "./errors": "./errors/index.js",
+    "./utilities": "./utilities/index.js",
+    "./*.js":"./*.js"
+  },
   "react-native": {
     "./dist/cache/inmemory/fixPolyfills.js": "./cache/inmemory/fixPolyfills.native.js"
   },
diff --git a/node_modules/@apollo/client/utilities/globals/graphql.js b/node_modules/@apollo/client/utilities/globals/graphql.js
index 62c0bf2..e26dfa6 100644
--- a/node_modules/@apollo/client/utilities/globals/graphql.js
+++ b/node_modules/@apollo/client/utilities/globals/graphql.js
@@ -1,4 +1,4 @@
-import { remove } from 'ts-invariant/process';
+import { remove } from 'ts-invariant/process/main.js';
 import { isType } from 'graphql';
 export function removeTemporaryGlobals() {
     isType(null);
diff --git a/node_modules/@apollo/client/utilities/package.json b/node_modules/@apollo/client/utilities/package.json
deleted file mode 100644
index 742faca..0000000
--- a/node_modules/@apollo/client/utilities/package.json
+++ /dev/null
@@ -1,9 +0,0 @@
-{
-  "name": "@apollo/client/utilities",
-  "main": "utilities.cjs.js",
-  "module": "index.js",
-  "types": "index.d.ts",
-  "sideEffects": [
-    "./globals/**"
-  ]
-}

Instead of generating package.json files for subdirectories, I'm just listing exports in the main package.json. This patch is not exhaustive, and I haven't tried it on a cjs setup.

I think this is more idiomatic node, too, WDYT?

@bennypowers bennypowers deleted the patch-1 branch July 28, 2021 19:17
@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
benjamn added a commit that referenced this pull request Jul 30, 2021
The root package.json file was neglected in #8396, since it isn't
generated in the same way as the other (nested) package.json files.

This commit puts `"type": "module"` in the _published_
`@apollo/client/package.json` file, which is generated at build time as
the file `dist/package.json`. The root `package.json` file that's
checked into the repository does _not_ contain `"type": "module"` (even
though putting it there would also put it in `dist/package.json`).
Attempting to enable ES module syntax at that level interfered pretty
severely with tools like ts-node, which we use to run various
config/*.ts scripts. Rewriting those scripts is a job for another time.

I'm happy to revisit this as the ecosystem progresses.
Venryx added a commit to Venryx/web-vcore that referenced this pull request Aug 6, 2021
@Venryx
Copy link

Venryx commented Aug 6, 2021

So I'm trying the latest beta, and it now has type: module for @apollo/client/core and such, which is great.

However, I'm finding I still cannot run my esmodule-based NodeJS v14 server with an import to @apollo/client/core and such, because lots of the sub-dependencies underneath @apollo/client are missing that type: module field.

Specifically:
tslib
zen-observable-ts
optimism
@wry/trie
@wry/equality
@wry/context
graphql-tag
symbol-observable

After using patch-package to modify the package.json files of all the packages above, I can finally use @apollo/client/core (and such) from NodeJS v14 in esm mode, without import errors. (my patch files: https://github.com/Venryx/web-vcore/tree/v0.0.17/patches)

What's ironic is that every one of the packages above provide esm versions. The problem is that NodeJS is not auto-recognizing the esm versions (even though they have fields like jsnext:main or exports: {import: ...} pointing to them); rather, NodeJS requires that the package.json file contain an additional type: module field, which then applies to all files under that path. I assume this is why the packages above have been reluctant to add the field: it would require them to either use .mjs extensions for all esm modules, or reorganize their output folder into multiple subfolders, so that differing package.json files (for the type field) can be used.

See the below for discussion in the NodeJS repo on the subject:

Anyway, I really wish NodeJS would respect/make-use-of the jsnext:main and exports: {import: ...} fields to automatically detect the esm modules. In the meantime though, I guess the solution is to manually patch 8+ third-party packages, while waiting for the package maintainers to reorganize their output folder(s) and use the additional type: module hint-flag.

@benjamn
Copy link
Member

benjamn commented Aug 9, 2021

@Venryx Either Apollo or I control all of those packages other than tslib and symbol-observable, so we can definitely get to work applying the same treatment from this PR to those packages. Would you mind opening a new issue to kickstart that work? Feel free to copy/paste what you already wrote. 🙏

Just so you know, the plan for now is to keep the "main" field pointing to a .cjs bundle for Node.js, though the new "type": "module" annotation allows Node.js to import .js files from @apollo/client/* correctly (as ESM). I don't think we're ready to make ESM the default Node.js experience for @apollo/client, in part because we've only just gotten to a point where all dependencies of @apollo/client/core export ESM in a bundler-friendly way (see #8266), and also because letting Node.js consume both CommonJS and ESM at the same time increases the risk of a dual package hazard.

@Venryx
Copy link

Venryx commented Aug 11, 2021

Yes, I will try to get back to this and open proper issues for each of the repositories. Can't at the moment since I'm on a (soon to be hit) deadline, but afterwards, it will be good to get it solved for the future.

benjamn added a commit that referenced this pull request Nov 16, 2021
benjamn added a commit that referenced this pull request Nov 16, 2021
benjamn added a commit that referenced this pull request Nov 18, 2021
From @apollo/client@3.5.0 until my recent PR #9073 (v3.5.3),
non-relative imported module identifiers like 'ts-invariant/process'
were rewritten during the build process (during 'npm run resolve') to
include a specific module with a file extension (not a directory),
producing in this example 'ts-invariant/process/index.js'.

That rewriting was the wrong choice in general (for example, it would
rewrite 'graphql' to 'graphql/index.mjs'), so PR #9073 disabled it for
all non-relative imports.

However, the specific replacement of 'ts-invariant/process' with
'ts-invariant/process/index.js' is still useful to prevent the strange
webpack 5-specific error

  The request 'ts-invariant/process' failed to resolve only because it was resolved as fully specified

which apparently happens because the
[resolve.fullySpecified](https://webpack.js.org/configuration/module/#resolvefullyspecified)
option is true by default now, and 'ts-invariant/process/package.json'
is ignored, effectively forbidding directory-style imports like
'ts-invariant/process' when the package.json of the enclosing package
has `"type": "module"` (which became true in Apollo Client v3.5, thanks
to #8396). App developers could configure resolve.fullySpecified
explicitly false, but we (the Apollo Client team) prefer a general
workaround.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants