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 exports to package.json #11570

Closed
wants to merge 11 commits into from

Conversation

andreialecu
Copy link
Contributor

@andreialecu andreialecu commented Feb 4, 2024

Updated the prepareDist.js script to generate an exports field, which will help ESM loaders properly identify which entry point to use.

Fixes #11569 #9925 #9976 #9048 #9156 #8218 apollographql/apollo-feature-requests#287

Generates:

"exports": {
    ".": {
      "types": "./index.d.ts",
      "import": "./index.js",
      "require": "./main.cjs"
    },
    "./index.js": {
      "types": "./index.d.ts",
      "default": "./index.js"
    },
    "./main.cjs": {
      "types": "./index.d.ts",
      "default": "./main.cjs"
    },
    "./package.json": {
      "default": "./package.json"
    },
    "./apollo-client.cjs": {
      "require": "./apollo-client.cjs"
    },
    "./cache": {
      "types": "./cache/index.d.ts",
      "import": "./cache/index.js",
      "require": "./cache/cache.cjs"
    },
    "./cache/*": "./cache/*",
    "./core": {
      "types": "./core/index.d.ts",
      "import": "./core/index.js",
      "require": "./core/core.cjs"
    },
    "./core/*": "./core/*",
    "./dev": {
      "types": "./dev/index.d.ts",
      "import": "./dev/index.js",
      "require": "./dev/dev.cjs"
    },
    "./dev/*": "./dev/*",
    "./errors": {
      "types": "./errors/index.d.ts",
      "import": "./errors/index.js",
      "require": "./errors/errors.cjs"
    },
    "./errors/*": "./errors/*",
    "./link/batch": {
      "types": "./link/batch/index.d.ts",
      "import": "./link/batch/index.js",
      "require": "./link/batch/batch.cjs"
    },
    "./link/batch/*": "./link/batch/*",
    "./link/batch-http": {
      "types": "./link/batch-http/index.d.ts",
      "import": "./link/batch-http/index.js",
      "require": "./link/batch-http/batch-http.cjs"
    },
    "./link/batch-http/*": "./link/batch-http/*",
    "./link/context": {
      "types": "./link/context/index.d.ts",
      "import": "./link/context/index.js",
      "require": "./link/context/context.cjs"
    },
    "./link/context/*": "./link/context/*",
    "./link/core": {
      "types": "./link/core/index.d.ts",
      "import": "./link/core/index.js",
      "require": "./link/core/core.cjs"
    },
    "./link/core/*": "./link/core/*",
    "./link/error": {
      "types": "./link/error/index.d.ts",
      "import": "./link/error/index.js",
      "require": "./link/error/error.cjs"
    },
    "./link/error/*": "./link/error/*",
    "./link/http": {
      "types": "./link/http/index.d.ts",
      "import": "./link/http/index.js",
      "require": "./link/http/http.cjs"
    },
    "./link/http/*": "./link/http/*",
    "./link/persisted-queries": {
      "types": "./link/persisted-queries/index.d.ts",
      "import": "./link/persisted-queries/index.js",
      "require": "./link/persisted-queries/persisted-queries.cjs"
    },
    "./link/persisted-queries/*": "./link/persisted-queries/*",
    "./link/retry": {
      "types": "./link/retry/index.d.ts",
      "import": "./link/retry/index.js",
      "require": "./link/retry/retry.cjs"
    },
    "./link/retry/*": "./link/retry/*",
    "./link/remove-typename": {
      "types": "./link/remove-typename/index.d.ts",
      "import": "./link/remove-typename/index.js",
      "require": "./link/remove-typename/remove-typename.cjs"
    },
    "./link/remove-typename/*": "./link/remove-typename/*",
    "./link/schema": {
      "types": "./link/schema/index.d.ts",
      "import": "./link/schema/index.js",
      "require": "./link/schema/schema.cjs"
    },
    "./link/schema/*": "./link/schema/*",
    "./link/subscriptions": {
      "types": "./link/subscriptions/index.d.ts",
      "import": "./link/subscriptions/index.js",
      "require": "./link/subscriptions/subscriptions.cjs"
    },
    "./link/subscriptions/*": "./link/subscriptions/*",
    "./link/utils": {
      "types": "./link/utils/index.d.ts",
      "import": "./link/utils/index.js",
      "require": "./link/utils/utils.cjs"
    },
    "./link/utils/*": "./link/utils/*",
    "./link/ws": {
      "types": "./link/ws/index.d.ts",
      "import": "./link/ws/index.js",
      "require": "./link/ws/ws.cjs"
    },
    "./link/ws/*": "./link/ws/*",
    "./react": {
      "types": "./react/index.d.ts",
      "import": "./react/index.js",
      "require": "./react/react.cjs"
    },
    "./react/*": "./react/*",
    "./react/components": {
      "types": "./react/components/index.d.ts",
      "import": "./react/components/index.js",
      "require": "./react/components/components.cjs"
    },
    "./react/components/*": "./react/components/*",
    "./react/context": {
      "types": "./react/context/index.d.ts",
      "import": "./react/context/index.js",
      "require": "./react/context/context.cjs"
    },
    "./react/context/*": "./react/context/*",
    "./react/hoc": {
      "types": "./react/hoc/index.d.ts",
      "import": "./react/hoc/index.js",
      "require": "./react/hoc/hoc.cjs"
    },
    "./react/hoc/*": "./react/hoc/*",
    "./react/hooks": {
      "types": "./react/hooks/index.d.ts",
      "import": "./react/hooks/index.js",
      "require": "./react/hooks/hooks.cjs"
    },
    "./react/hooks/*": "./react/hooks/*",
    "./react/internal": {
      "types": "./react/internal/index.d.ts",
      "import": "./react/internal/index.js",
      "require": "./react/internal/internal.cjs"
    },
    "./react/internal/*": "./react/internal/*",
    "./react/parser": {
      "types": "./react/parser/index.d.ts",
      "import": "./react/parser/index.js",
      "require": "./react/parser/parser.cjs"
    },
    "./react/parser/*": "./react/parser/*",
    "./react/ssr": {
      "types": "./react/ssr/index.d.ts",
      "import": "./react/ssr/index.js",
      "require": "./react/ssr/ssr.cjs"
    },
    "./react/ssr/*": "./react/ssr/*",
    "./testing": {
      "types": "./testing/index.d.ts",
      "import": "./testing/index.js",
      "require": "./testing/testing.cjs"
    },
    "./testing/*": "./testing/*",
    "./testing/core": {
      "types": "./testing/core/index.d.ts",
      "import": "./testing/core/index.js",
      "require": "./testing/core/core.cjs"
    },
    "./testing/core/*": "./testing/core/*",
    "./utilities": {
      "types": "./utilities/index.d.ts",
      "import": "./utilities/index.js",
      "require": "./utilities/utilities.cjs"
    },
    "./utilities/*": "./utilities/*",
    "./utilities/subscriptions/relay": {
      "types": "./utilities/subscriptions/relay/index.d.ts",
      "import": "./utilities/subscriptions/relay/index.js",
      "require": "./utilities/subscriptions/relay/relay.cjs"
    },
    "./utilities/subscriptions/relay/*": "./utilities/subscriptions/relay/*",
    "./utilities/subscriptions/urql": {
      "types": "./utilities/subscriptions/urql/index.d.ts",
      "import": "./utilities/subscriptions/urql/index.js",
      "require": "./utilities/subscriptions/urql/urql.cjs"
    },
    "./utilities/subscriptions/urql/*": "./utilities/subscriptions/urql/*",
    "./utilities/globals": {
      "types": "./utilities/globals/index.d.ts",
      "import": "./utilities/globals/index.js",
      "require": "./utilities/globals/globals.cjs"
    },
    "./utilities/globals/*": "./utilities/globals/*"
  }

@andreialecu andreialecu requested a review from a team as a code owner February 4, 2024 11:21
Copy link

netlify bot commented Feb 4, 2024

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 992ae9a

Copy link

changeset-bot bot commented Feb 4, 2024

🦋 Changeset detected

Latest commit: 992ae9a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@andreialecu
Copy link
Contributor Author

andreialecu commented Feb 4, 2024

Some potential issues are remaining. (addressed in later commits/see later comments)

If users have been adding deep imports to specific files, those will stop working.

I noticed some users have been working around this issue for a while. Here's one example:

https://github.com/yavin-dev/framework/blob/cd2b41054f723789600f5de4ae7d4f16f11584b5/packages/client/src/plugins/elide-apollo-client.ts#L5-L11

We can also list the .cjs exports like:

    "./cache/cache.cjs": "./cache/cache.cjs",

However, this seems to trip the attw (Are the types wrong?) test for node 10 on these .cjs files. I'd need to add a workaround to make it ignore all these .cjs files. The file I linked above has some comments on how the types are already broken, so it seems to be expected.

@andreialecu
Copy link
Contributor Author

@andreialecu
Copy link
Contributor Author

andreialecu commented Feb 4, 2024

Update: I added wildcard exports, which should address my previous concerns.

Also added some more tests to validate this. The deep imports work, and now the esm imports also point where they should without needing to use index.js

This should be ready for review. Perhaps a maintainer can create a preview build that would be easy to swap in.

@andreialecu
Copy link
Contributor Author

andreialecu commented Feb 5, 2024

Hey @phryneas - found your comment in:
#9976 (comment)

However, I believe this should not be a breaking change because the wildcards address any potential deep imports.

I have added some tests for the deep imports, and they work properly. If we can identify other imports people are using, we can also add them to the tests.

I have also verified the modified package.json in a pretty big react-native and an angular project, and there were no compilation or runtime issues.

@phryneas
Copy link
Member

phryneas commented Feb 5, 2024

Hi @andreialecu,
thank you for the PR.

I'm sorry though, but everything I said there still stands.
I know that it doesn't look like it, and I appreciate that you have been very thorough here, but no matter how thorough we are in the end - the pure existence of an exports field will break the builds of some of our consumers. And we'll always only know after that fact.
Introducing a change like that is fine in a major, but not in a minor.

That said: we are targeting a major this year, and bundle changes as well as an exports field are on the top of the list for that. This change will come. But not right now.

Unfortunately, the changes you made here (as any other exports pull request) will be superseded by the changes we will make then, because we will have to move around a lot of our bundles and create new artifacts (including e.g. .d.cts and .d.mts files), so I'm very sorry, but this is something we cannot do now and also something that we will have to completely re-do ourselves when it's the time for it.
As a consequence, I'm going to close this PR. I'm very sorry and really appreciate the time you put into this!

@phryneas phryneas closed this Feb 5, 2024
@andreialecu
Copy link
Contributor Author

andreialecu commented Feb 5, 2024

I understand the concern for breaking changes, can you point out to a case you think will be broken by these changes, besides anecdotally?

The previous PRs did not handle this properly. I suggest you do a thorough review to see if things are different here.

Can you point me to a specific repo somewhere on github that will be broken by these changes? Or, could you create a preview build that people can test, and that they can provide feedback about whether it breaks anything?

I don't see why this couldn't be a temporary solution until then.

@phryneas
Copy link
Member

phryneas commented Feb 5, 2024

To be frank, I can not.

I can point out half a dozen packages that have been broken in various ways by changes like this.
I can tell you anecdotally that there is a good chance that certain versions of jest, vitest and at lease some versions of webpack will pick up different bundles the moment an exports field is found.
I can tell you about a lot of frustration with dual package hazard that we and others have experienced already, that would probably become a lot worse based on a change like this.
I can tell you about dozens of discussions on this topic I had with other package maintainers that have been burned by this.

But I cannot point a finger at "what" will break, I can only say "it's extremely likely that something will break for a certain percentage of our user base".
And looking at the download numbers of this package, that will be thousands of users.

If we were to publish this change, we couldn't go back on it, as undoing this change would be a breaking change in itself.

I know that this is not an ideal situation, but 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.

@andreialecu
Copy link
Contributor Author

andreialecu commented Feb 5, 2024

I know that this is not an ideal situation, but 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.

Agree, and that is why I explicitly proposed creating a preview build for people to test.

However, in other packages, that was caused directly because some exports that people were deep importing were not re-exported. For example there's an explicit mention of how this would be added in a non-breaking way to eslint here: eslint/eslint#13654 (comment) - just by making sure everything is still reachable.

This exports issue has plagued the community for years, resulting in suboptimal code in many libraries. It will open up a way to remove many ugly hacks.

Just take a look at this for example: https://github.com/yavin-dev/framework/blob/cd2b41054f723789600f5de4ae7d4f16f11584b5/packages/client/src/plugins/elide-apollo-client.ts#L5-L11

There is no reason this will change anything and I believe the fears are exaggerated.

There's no "other bundle" to pick because the same files are referenced by the exports as what people might've been using as a workaround. The wildcard exports allow passthrough to the deep imports. There's no "rewriting" or "aliasing" going on, if there were, then I would've agreed.

(perhaps @hi-ogawa, the Vite maintainer who originally pointed me to this issue in Apollo could chime in on whether this sort of change can cause any problems)

In my opinion, it doesn't seem good to close this issue so superficially.

@phryneas
Copy link
Member

phryneas commented Feb 5, 2024

I'm not closing this issue superficially without looking into it deeper, and I'm really sorry if that's the impression you have.
I'm closing it because we had this discussion over here about five times, and because I've spent a lot of time thinking about this problem and discussing it not only with our team, but also with a lot of external maintainers.

I'm not only an Apollo Client maintainer, but also a Redux maintainer, and I've followed @.acemarke on his path to get Redux Toolkit to work correctly in every environment closely (as in, we regularly chatted about it). He has blog articles, a conference talk and a few podcast episodes about it if you're interested.
That took him over a year, and after we released, there were still people complaining about breakages.

However, in other packages, that was caused directly because some exports that people were deep linking to were not re-exported.

As I said, this is not the only problem. There are bundlers around that completely change what they import based on the existence of an exports field. No exports field? It uses the CommonJS import. An exports field? For some reason, the ESM import seems more interesting now (even though there would also be a CommonJS one in the exports field). Oh no, now some loader that worked before doesn't work anymore, because it doesn't recognize the syntax.


On your vite.js problem, quoting from the other thread:

Unfortunately resolve.alias is not expected to work to patch how apollo-angular imports @apollo/client/core.

Right now, the workaround is to directly import .js files, and it might be worth issuing a PR to apollo-angular to import @apollo/client/core/index.js instead of @apollo/client/core. That should be a much safer change.

PS: The only workaround we could do right now would be to create some @apollo/client-esm-compat library that contains only a package.json with an exports field - and we've considered that. But unfortunately, that wouldn't solve situations like this, where a dependency imports from @apollo/client. I believe the most pragmatic thing you can do here would probably be using patch-package or something similar.

@hi-ogawa
Copy link

hi-ogawa commented Feb 5, 2024

Sorry if I gave some confusion as an outsider of apollo/angular ecosystem.
I totally understand Apollo team's perspective and I would also expect that introducing exports is not something a library with a huge ecosystem can do easily.

As far as I understand, exports is not strictly necessary for NodeJS esm compatibility. As @phryneas suggested, I think the user side (in this case apollo-angular) should be able to adapt esm usage by using import "@apollo/client/core/index.js". It looks like typescript can still provide types for such import https://stackblitz.com/edit/vitest-dev-vitest-zjsqtx?file=src%2Frepro-types.ts

Again I'm not familiar with the ecosystem, but to me, I thought this could be a documentation issue first since import "@apollo/client/core" is used here https://www.apollographql.com/docs/react/migrating/apollo-client-3-migration/#using-apollo-client-without-react. Perhaps this could be fixed or at least adding some warning for esm resolution could help?

@phryneas
Copy link
Member

phryneas commented Feb 5, 2024

Again I'm not familiar with the ecosystem, but to me, I thought this could be a documentation issue first since import "@apollo/client/core" is used here apollographql.com/docs/react/migrating/apollo-client-3-migration/#using-apollo-client-without-react. Perhaps this could be fixed or at least adding some warning for esm resolution could help?

Good call! This documentation is probably older than the exports field (our ESM bundles are a lot older than it, at least 😅 ). I'll bring it up in our team meeting.

@andreialecu
Copy link
Contributor Author

andreialecu commented Feb 5, 2024

Thank you for the context.

I am leaving some notes here for posterity after doing a deeper dive into the issues seen by Redux Toolkit and Immer:

This blog post explains the saga of updating redux-toolkit for ESM support: https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons/

Some notes:

My key takeaway is that most of the issues above were related to using the .mjs extension instead of .js or type: "module" (which is already there), and not related to exports being present or not:

Also, FWIW based on my previous comment:

I have also verified the modified package.json in a pretty big react-native and an angular project, and there were no compilation or runtime issues.

The Angular project uses Webpack 5, and the react-native project uses Metro. There were no issues from these changes in either project.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERR_UNSUPPORTED_DIR_IMPORT when importing @apollo/client/core
3 participants