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 export specification #9697

Closed
wants to merge 5 commits into from
Closed

Conversation

tobiasdiez
Copy link

@tobiasdiez tobiasdiez commented May 8, 2022

Define Package entry points using the exports field in package.json. There might be additional paths that you want to expose and I'm not sure if the utilities should be exposed by this package.json file or by its own one.

Fixes #9048 and probably #9938, #9156, #8218, apollographql/apollo-feature-requests#287

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

@apollo-cla
Copy link

@tobiasdiez: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@jpvajda jpvajda added the 🏓 awaiting-team-response requires input from the apollo team label May 13, 2022
Comment on lines 16 to +35
"main": "./dist/main.cjs",
"module": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
".": {
"import": "./dist/index.js",
"require": "./dist/main.cjs"
},
"./core": {
"import": "./core/index.js",
"require": "./core/core.cjs"
},
"./link/error": {
"import": "./link/error/index.js",
"require": "./link/error/error.cjs"
},
"./utilities/policies/pagination": {
"import": "./utilities/policies/pagination.js"
}
},
Copy link
Contributor

@rosskevin rosskevin Jun 3, 2022

Choose a reason for hiding this comment

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

To be compliant, I read the typescript handbook as the following (remove commented lines, add in types, and note the wrong published module path has not dist). Also note I added in the link/core module:

  // -"main": "./main.cjs",
  // -"module": "./index.js",
  // -"types": "./index.d.ts",
  "exports": {
    ".": {
      "types": "./index.d.ts",
      "import": "./index.js",
      "require": "./main.cjs"
    },
    "./core": {
      "types": "./core/index.d.ts",
      "import": "./core/index.js",
      "require": "./core/core.cjs"
    },
    "./link/core": {
      "types": "./link/core/index.d.ts",
      "import": "./link/core/index.js",
      "require": "./link/core/error.cjs"
    },
    "./link/error": {
      "types": "./link/error/index.d.ts",
      "import": "./link/error/index.js",
      "require": "./link/error/error.cjs"
    },
    "./utilities/policies/pagination": {
      "types": "./utilities/policies/pagination.js",
      "import": "./utilities/policies/pagination.js"
    }
  },

I will note however, that while I think the above exports are correct according to the handbook, that I still receive errors when attempting to use option "moduleResolution": "nodenext", but it is fine when using the more typical node resolution. (I believe this to be a bug with typescript and export * from using nodenext resolution and I'm working on a test case for them).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the input. I believe though that the dist prefix is necessary, since that's where the files are in the distributed package. Feel free to add your other changes as github suggestions, then I will merge them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tobiasdiez I have confirmed that dist in not in the published package. See https://unpkg.com/@apollo/client@3.6.6/dist or
tsconfig_react-lib_json_—_ts-esm-workspaces

Copy link
Author

Choose a reason for hiding this comment

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

It seems like you are right and there is indeed no dist folder. But the dist path is akso in "main" and "module", strange...

Copy link
Contributor

Choose a reason for hiding this comment

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

These paths must be rewritten during the build process. If you yarn patch @apollo/client and open an editor, you can see the produced paths more easily.

@rosskevin
Copy link
Contributor

Here is a reproduction of apollo failing as a pure ESM library import due to what I believe is the lack of proper exports (I could be wrong). Note that switch typescript to "moduleResolution": "node" instead of "moduleResolution": "nodenext" allows this to succeed, which supports the argument that the problem is exports based.

https://github.com/rosskevin/ts-esm-workspaces/tree/apollo-exports

@Mordred
Copy link

Mordred commented Jun 30, 2022

Here is my not working use case https://github.com/Mordred/apollo-client-esm

I created new project and added "type": "module" to package.json. NodeJS throws a SyntaxError that @apollo/client is CommonJS module.

It would be nice to have exports defined - but do not forget to add ./link/schema, ./link/ws and more which are not currently in this PR but are in the docs (e.g. https://www.apollographql.com/docs/react/api/link/apollo-link-schema/)

@tobiasdiez
Copy link
Author

@Mordred Thanks for the suggestion. Could you provide the other exports as PR review suggestions, so I can easily commit them to this PR in your name?

@jpvajda @MrDoomBringer I would very much appreciate if you could find the time to review this PR.

@jpvajda
Copy link
Contributor

jpvajda commented Sep 8, 2022

👋 @tobiasdiez I noticed a weird Circle CI error on this PR, so I'm curious what might be occurring here. Jyst adding a few people from the team to check this out for you. @bignimbus @hwillson

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.Please sign out of CircleCI and log back in with your VCS before triggering a new pipeline.

@bignimbus
Copy link
Contributor

Thanks for the tag @jpvajda and thanks for your contribution @tobiasdiez! I haven't been able to narrow down the issue with CircleCI - @tobiasdiez what do you see when you click the "Details" link next to "CircleCI Pipeline?"

I was able to get a build started by pushing a branch with these changes. See https://app.circleci.com/pipelines/github/apollographql/apollo-client/16912/workflows/ae50495f-993e-4359-94f8-b3b37dcd2502/jobs/93836 - it looks like there are some failing tests regarding modules:

     Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './apollo-client.cjs' is not defined by "exports" in /home/circleci/project/scripts/memory/node_modules/@apollo/client/package.json
     Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './react' is not defined by "exports" in /home/circleci/project/scripts/memory/node_modules/@apollo/client/package.json

Given @Mordred's comment, I think we need to take a closer look at our test coverage to ensure that all entry points are covered.

@tobiasdiez
Copy link
Author

@bignimbus Thanks for investigating. I get a similar message on CircleCI:

Could not find a usable config.yml, you may have revoked the CircleCI OAuth app.Please sign out of CircleCI and log back in with your VCS before triggering a new pipeline.

It's also not working if I try to create a new PR: #10086. So maybe its best if you (or someone else) creates a new PR with the changes.

@bignimbus
Copy link
Contributor

Thanks for trying again @tobiasdiez 🙏🏻 I would definitely like to figure out what might be going on. Regardless of whether it's a broader issue with our CircleCI config or something happening in your specific account, I'd like to make sure you're fully able to contribute to our community. Would it be ok for me to reach out directly via your Twitter DMs next week and try a couple of debug steps? Completely fine if you'd prefer not to, whatever works for you!

@tobiasdiez
Copy link
Author

I've now created a circleci account (via github oauth) and it seems like this did the job. Strange though...

@MrDoomBringer
Copy link
Contributor

MrDoomBringer commented Sep 12, 2022

Hey @tobiasdiez ! Thanks for the ping on this.

The question of entrypoint coverage in this thread looks to me to be a bit of a challenge. To truly prevent a breaking change, we'd need to cover (just about) every export statement in @apollo/client/..., rather than only the module entrypoints defined in config/entrypoints.js and otherwise mentioned in the docs.

That would look something like this exports definition ...

"exports": {
    "./testing": {
      "import":"./testing/index.js",
      "require":"./testing/testing.cjs"
    },
    "./testing/core": {
      "import":"./testing/core/index.js",
      "require":"./testing/core/core.cjs"
    },
      "./testing/*": "./testing/*.js",
      "./testing/*.js": "./testing/*.js"
  }

... to support the public entrypoints we currently have in @apollo/client/testing, for example:

// foo.js
// > node --experimental-specifier-resolution=node foo.js
import * as a from '@apollo/client/testing/core/mocking/mockClient'
import * as b from '@apollo/client/testing/core/mocking/mockClient.js'
import * as c from '@apollo/client/testing/core/itAsync'
import * as d from '@apollo/client/testing/core/itAsync.js'
import * as e from '@apollo/client/testing/core'
import * as f from '@apollo/client/testing/react/MockedProvider'
import * as g from '@apollo/client/testing/react/MockedProvider.js'
import * as h from '@apollo/client/testing'

Even taking advantage of node's subpath export patterns I worry how maintainable our package.json will be after covering every public entrypoint we have right now.

Also, doing all this might be avoiding what the team at nodejs.org is trying to get us to do:

allowing module authors to clearly define the public interface for their package.

It might be that the correct move here includes targeting a later release (Maybe 4.0?), then some planning on what we want our public interface to be and reorganizing our codebase to make that interface more easily accessible.

I'm not yet totally certain on my answer here, please let me know if I missed something.

All the best,
Emmanuel, Intern :-)

@MrDoomBringer MrDoomBringer self-assigned this Sep 12, 2022
@tobiasdiez
Copy link
Author

I guess it depends on what you define as the public interface. For example, direct imports of js files like '@apollo/client/testing/core/mocking/mockClient.js' I would consider as a private implementation detail whose public interface is '@apollo/client/testing/core/mocking/mockClient'.

But yes, in general, I agree it would be nice if apollos interface could be streamlined and many deep imports combined (e.g. @apollo/client/testing being the only entrypoint). But let's not do this as part of this PR.

@rosskevin
Copy link
Contributor

rosskevin commented Oct 27, 2022

@MrDoomBringer perhaps a breaking change is necessary. I agree that the apollo packaging is quite unconventional, but regardless, people using ESM are broken because apollo packages are not providing an updated picture of their package via the exports specification. I'm for a major bump if the dist can get cleaned up and the exports specified appropriately.

For what it is worth, here is my current yarn patch - I know that it is incomplete (indeed I already noted the lack of cjs file in the last entry)...but it is getting me by (so far).

diff --git a/package.json b/package.json
index 3a2bf8a3ead2ca597564f8fbfafa8af78c385a36..d4aee2f69d4e77e8bcf836265ce8bc2c2b10958a 100644
--- a/package.json
+++ b/package.json
@@ -16,6 +16,37 @@
   "main": "./main.cjs",
   "module": "./index.js",
   "types": "./index.d.ts",
+  "exports": {
+    ".": {
+      "types": "./index.d.ts",
+      "import": "./index.js",
+      "require": "./main.cjs"
+    },
+    "./core": {
+      "types": "./core/index.d.ts",
+      "import": "./core/index.js",
+      "require": "./core/core.cjs"
+    },
+    "./link/core": {
+      "types": "./link/core/index.d.ts",
+      "import": "./link/core/index.js",
+      "require": "./link/core/error.cjs"
+    },
+    "./link/error": {
+      "types": "./link/error/index.d.ts",
+      "import": "./link/error/index.js",
+      "require": "./link/error/error.cjs"
+    },
+    "./testing": {
+      "types": "./testing/index.d.ts",
+      "import": "./testing/index.js",
+      "require": "./testing/testing.cjs"
+    },
+    "./utilities/policies/pagination": {
+      "types": "./utilities/policies/pagination.d.ts",
+      "import": "./utilities/policies/pagination.js"
+    }
+  },
   "sideEffects": false,
   "react-native": {
     "./dist/cache/inmemory/fixPolyfills.js": "./cache/inmemory/fixPolyfills.native.js",

@revmischa
Copy link

revmischa commented Nov 21, 2022

I think it makes total sense to simplify the public interface and release as 4.0.0
All of the @apollo/client code is getting bundled in both the client and server-side code of my application which makes web pages slower to load and lambda graphql resolver functions slower in cold starts. If this can help, it will be a great improvement for my users.
Ref: apollographql/apollo-feature-requests#287 (comment)

@tobiasdiez
Copy link
Author

How should we proceed here? Is there something I can do to move this forward?

@phryneas
Copy link
Member

phryneas commented Mar 8, 2023

@revmischa This is something we cannot do without a 4.0, and we will not do a 4.0 just for this. Reviewing the whole bundling setup is something we will be doing for the next major, especially with ESM in mind.

Unfortunately, there is still no generally-agreed way of serving ESM without breaking some build or bundling tools. For example, see over in the Redux repo, where we are trying to do this at this moment. Every change published in an alpha generates a bug report that something is broken.

At this point, it's probably best that you use something like the patch suggested in #9697 (comment) until we get ready for the next major (and someone hopefully figures this problem space out before that).

@@ -16,6 +16,23 @@
"main": "./dist/main.cjs",
"module": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
".": {
"import": "./dist/index.js",

Choose a reason for hiding this comment

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

Without "type": "module" set, .js is interpreted as CJS, not ESM. You need to use .mjs to refer to ESM in a CJS package (which is the default).

@JasonKleban
Copy link

Related #9976

@phryneas
Copy link
Member

I just mentioned this over in #10620 - at the point where we get to this change (definitely in the next major release!), we will likely have to adopt tooling that will autogenerate these exports fields.

As I'm doing some housekeeping, I'm going to close this PR for now - that definitely doesn't mean that we won't do this though!
Really appreciate the PR and bringing this up 👍

@phryneas phryneas closed this Sep 14, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 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.

yarn build/start import @apollo/client/core/core.cjs.js?