Support Apollo Client v3

[hoangvvo]

This PR use the new @apollo/client.

Also since rewriteURIForGET is exported in @apollo/client/link/http/rewriteURIForGET, we can support GET requests

Fixes #174 .

[hoangvvo]

Should we also offer ES6 module version for tree-shaking?

We can have main pointed to the transpiled version while module (supported by webpack) pointed to our src.

[jaydenseric]

Thanks for taking a look at this! Hopefully the issue of rewriteURIForGET not being exported as CJS can be rectified on Apollo's end, then we can look at merging once Apollo Client v3 is out of pre-release.

[jaydenseric]

Should we also offer ES6 module version for tree-shaking?

This package used to have a module field, but it was removed in v9:

https://github.com/jaydenseric/apollo-upload-client/releases/tag/v9.0.0

I only intend to add it back if we can get it working properly with .mjs and native ESM in Node.js.

For a modern example of a package with a module field, see:

https://unpkg.com/browse/[email protected]/lib/
https://github.com/jaydenseric/extract-files/blob/master/babel.config.js

We can have main pointed to the transpiled version while module (supported by webpack) pointed to our src.

The module field should never point to untranspiled source files; the only difference between exported CJS and ESM should be the module format.

[hoangvvo]

What concerns me is the way they initialize the client using a uri key here. The README should address this I believe. So I guess apollo-boost makes a "come-back" in a different form 🤣

From https://www.apollographql.com/docs/react/api/link/introduction/:

By default, Apollo Client uses Apollo Link's HttpLink to send GraphQL operations to a remote server over HTTP. Apollo Client takes care of creating this default link, and it covers many use cases without requiring additional customization.

[hoangvvo]

@adriencohen , based on your earlier work in #196 what do you think about all the extra peer dependencies @apollo/client v3 introduces:

https://github.com/apollographql/apollo-client/blob/287aa465d56fea7d2171533921e73b78caac26a7/package.json#L57-L66

Should we match these pretty hefty @apollo/client peerDependencies and peerDependenciesMeta exactly in this PR, even though we don't require any of them directly?

What about having @apollo/client as a peer dependency? Would yarn strict mode still complain if we do not have those peer dependencies? It also makes more sense for @apollo/client to be a peer dependency based on the definition of one:

In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host. This is usually referred to as a plugin.

[jaydenseric]

@hoangvvo

What about having @apollo/client as a peer dependency?

It should be a regular dependency as this package uses require to import from @apollo/client like any other dependency.

In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host. This is usually referred to as a plugin.

An example of a plugin would be a Babel plugin that doesn't actually import anything from Babel, but depends on Babel injecting certain helpers, context, etc. into the plugin function.

Regarding the react optional peer dependency; I'm not exactly sure how all package managers work but surely since it's optional they won't complain if we don't add it here?

[hoangvvo]

@hoangvvo

What about having @apollo/client as a peer dependency?

It should be a regular dependency as this package uses require to import from @apollo/client like any other dependency.

In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host. This is usually referred to as a plugin.

An example of a plugin would be a Babel plugin that doesn't actually import anything from Babel, but depends on Babel injecting certain helpers, context, etc. into the plugin function.

Regarding the react optional peer dependency; I'm not exactly sure how all package managers work but surely since it's optional they won't complain if we don't add it here?

Thanks for your explanation. It makes sense.

I'm not sure why some packages like express-graphql or apollo-server have graphql as their peer dependencies even though each of them requires it.

[jaydenseric]

@hoangvvo

I'm not sure why some packages like express-graphql or apollo-server have graphql as their peer dependencies even though each of them requires it.

That's because graphql can't work properly if there are multiple versions in node_modules (internally it does instanceof checks against its own classes, etc.), and a lot of packages depend on it, so to prevent one slow to update one from holding up the show the community decided at some point to always rely on the user installing it in their project as the source of truth. This is still imperfect; whenever you update graphql to a new major version things often break in node_modules until they are updated to support the new API. Just like how it's a nightmare everytime Babel core has a major release with breaking changes. A benefit with graphql as a peer dependency is that if a project dependency that's slow to update its required graphql version happens by pure luck to not be broken by a major graphql update it will be forced to use the new graphql peer dependency instead of installing the old version were it a dependency.

Both approaches will work, with pros and cons. Here is my decision process when installing:

1. If there are no imports of the dependency, but your package is intended to work with a specific version of it as a plugin; peer dependency for sure.
2. If the community has a clearly established convention to always install it as a peer dependency (except at the root project level), follow the convention so as to not nullify the collective benefit of that approach.

If you are deciding to create a community convention, my thought process would be: Could multiple versions of this package coexist in node_modules? If yes, not a peer dependency. If no, I would still think pretty hard about avoiding peer dependencies as it adds more complexity to documentation, etc. as users have to take more responsibility and it only takes one package to not follow the convention to nullify much of the benefit to a user. For example, ESLint has been trying to work out a better configuration system for years to avoid the hassle users go through when installing sharable configs; each plugin used by the config has to be installed by the user in their project.

[PowerKiKi]

Should we also offer ES6 module version for tree-shaking?

Angular is now spitting warning when using this lib because it uses CommonJS module. So from my point of view it would be extra nice to support ESM at the same time as Apollo 3.

WARNING in XXXX depends on 'apollo-upload-client'. CommonJS or AMD dependencies can cause optimization bailouts.
For more info see: https://angular.io/guide/build#configuring-commonjs-dependencies

[jaydenseric]

@PowerKiKi I'm going to work on ESM outside of this PR, but it will be hard to have a good outcome because @apollo/client is very flawed.

There is a lot to explain. I have been moving all my packages to support all of this at the same time:

1. Support consuming via ESM, properly so import works in Node.js without transpillation.
2. Support consuming via CJS and require.
3. Ensure only the functionality needed can be required or imported (both named and deep imports). Deep imports avoid the need for tree shaking entirely and guarantee you only bundle what is needed.
4. Ensure that no matter how you pull in a given API (require, import, named imports, deep default imports), it will not cause duplication in a bundle, no matter how many different ways you do it in your codebase. This is know as avoiding the dual package hazard.
5. Ensure that private package code literally can't be imported from outside the package. This allows us to publish updates with semver patch or minor versions that radically change implementation code without worrying it will be a breaking change for consumers doing deep imports of internal code that isn’t a public API.
6. Support back to Node.js v10.x.
7. Support Webpack (at least v5 optimally as it supports the package exports field and conditional entries, but other versions should work).

The only way to tick all those boxes is like this:

https://github.com/jaydenseric/graphql-upload/blob/a3b7b31406aa47567ced0b60eea1cd764db8932c/package.json#L32-L40

You can see documented all the different ways parts of the public API can be consumed:

https://github.com/jaydenseric/graphql-upload#class-graphqlupload

Now, @apollo/client as published doesn't really tick most of the boxes. The most obvious problem is that they bundle what is published, which is an huge antipattern. They actually have multiple bundles in the same package, with duplicated code! E.g. if in your project you do both const { HttpLink } = require('@apollo/client') and const { HttpLink } = require('@apollo/client/link/http') you will get two separate HttpLink classes in your project bundle. apollo-upload-client should work with require or import in Node.js without transpilation, so it's not an option to use deep imports to retrieve the specific APIs we need as they are ESM (not Node.js compatible ESM; files don't have the .mjs extension and import specifiers don't contain file extensions), and CJS is not available for specific APIs without pulling in entire CJS bundles containing a bunch of other redundant stuff.

[PowerKiKi]

Thanks for the detailed answer and your work on those lib. I was not aware that @apollo/client had the issues you described.

[jaydenseric]

🙌

[adriencohen]

@adriencohen , based on your earlier work in #196 what do you think about all the extra peer dependencies @apollo/client v3 introduces:

https://github.com/apollographql/apollo-client/blob/287aa465d56fea7d2171533921e73b78caac26a7/package.json#L57-L66

Should we match these pretty hefty @apollo/client peerDependencies and peerDependenciesMeta exactly in this PR, even though we don't require any of them directly?

I think what you have done in the PR is right, I'll report it to you if yarn2 yells at me

[Vultraz]

Will there be a release with this soon?

[hoangvvo]

Thanks for the detailed answers above to our questions! It's really helpful.

[jaydenseric]

This has been published in v14.0.0 🚀

I still feel pretty unhappy about a few things:

1. The @apollo/client and bundle size and code duplication (see Support Apollo Client v3 #175 (comment)), but that's something that can be worked on at Apollo's end to benefit the whole ecosystem. Someone feel free to raise an issue over there! If they can provide deep imports to CJS for individual elements of the API, we can then update our imports here accordingly and resolve the issue.
2. I don't think babel-plugin-graphql-tag is processing gql when it's imported from @apollo/client instead of the usual graphql-tag, see Does this work for gql imported from @apollo/client? gajus/babel-plugin-graphql-tag#41 . We updated the example code in apollo-upload-client to import gql from @apollo/client, but if that doesn't pan out to be good advice we can revert it in a patch release.

I've updated the example app, but please note point 2 above.

Next up is to add tests in a patch release (see #204), then we can add support for GET requests (with tests) in a minor release (see #151).

[nelsonpecora]

Is there anything people can help with around supporting GET requests? It looks like apollographql/apollo-client#6593 might have unblocked that, but from my reading of the discussion around apollo's bundling it sounds like it might be more complicated that I'm assuming. 😅

[jaydenseric]

@nelsonpecora I've been working full time the past few days on adding tests (#204), but got a little stuck on aspects of the abort controller signals. If I can't figure it out today, I will push up all the other tests anyway.

If I have time remaining today I will add tests and support for GET (#151). The actual GET related helpers are available to us now from @apollo/client, so there are no technical blockers.

If I run out of time (I already took 3 days off paid work this week for this; I have to return to my contract work for Thursday and Friday), hopefully the community can raise a PR. From here on out only PRs with appropriate test coverage will be reviewed. Having tests will make it much easier to review and accept contributions!

[iprokic-equinix]

Looks good