0.10.0

[fbartho]

Tracking PR for the 0.10.0 release!

What's Changed

• Breaking: v0.10.0 drops support for ApolloClient v3!
• Quick & dirty Apollo v4 compat #330 by @jackstudd
• Updates to all the dependencies for the modern era

Known Issues

• There's some sort of packaging issue around ESModules & Source-mapping, possibly related to webpack. -- This is a blocker for shipping.

[jerelmiller]

Did a quick review and pointed out a few (hopefully) helpful things.

Appreciate you moving this forward! This has been a great addition to the community and one that was super influential early in my career. Awesome to see continued improvements 😎

[jerelmiller]

If I may, I would suggest adding support for the exports field in this release if you can as some bundlers treat the presence of that property as a breaking change. We waited to introduce it until 4.0 for this reason. Since v4 supports it, it should be safe to add exports as well in this package 🙂.

[fbartho]

I'm not an expert at CJS/ESM nonsense 😉, what should my exports config look like?

These are the files in our built directory:

bundle.umd.js
bundle.umd.js.map
index.d.ts
index.js
index.js.map
LICENSE
package.json
README.md
restLink.d.ts
restLink.js
restLink.js.map
schema.graphql
utils

[jerelmiller]

By the way, the RxJS from helper will turn promises into observables, so if you wanted to use that here, that might be helpful. You can combine this with catchError if you want to handle certain errors and either emit a value or rethrow (like you do with the catch block below).

Totally fine to leave it this way, but you might find it helpful to flatten this out a bit 🙂

Rough idea of what this might look like (obvioiusly feel free to play around with the pipeline if you prefer this approach)

obs.pipe(
  mergeMap(({ data, errors }) => {
    return from(graphql(...))
  }),
  tap(() => setContext(...)),
  map((data) => ({ data, errors })),
  catchError(err => {
    if (err.name) === 'AbortError')) {
      return
    }

    if (err.result && err.result.errors) {
      return of(err.result);
    }

    throw err
  })
)

[fbartho]

I appreciate this, however I'm not personally motivated to make this change. I'm only a very rare user of RXJS myself, so since the tests pass, I wasn't looking to mess with functioning code.

I'd absolutely accept a PR that changes this, however!

[jerelmiller]

By the way, it might help to create a helper that will prepopulate the client for you. See our link helpers for inspiration. We import that and alias to execute to make these calls a bit nicer.

We should probably publish that as an official test utility, but until then, that might be helpful 🙂

[fbartho]

That would have been smart. I'm happy to accept a PR for that, but it's outside of my time-budget for this project right now.

(I'm a startup founder with very little time for OSS right now, and I just felt guilty that this didn't work on ACv4 and it had been so long since the last release the #330 felt like such a good starting point that I spent my lunch and dinner breaks on this)

[fbartho]

@jerelmiller I've implemented the suggested changes that I didn't otherwise ask for help on! Please feel free to give this PR an approval if you think it's ready, or let me know what else you think is a blocker.

It looks like the Apollo permissions on this repo are slightly borked, and despite being the only maintainer, I no longer have the ability to merge a PR without an approving review. (Is that something you can help fix? -- I'm not sure if I can force push to master, but that'll be a blocker soon assuming we get positive feedback on these RCs)

[fbartho]

@jerelmiller oh! and thanks! haha

[jerelmiller]

FYI I'm still planning to open a PR to address those final few things. I should be able to get to that this afternoon 🙂

[fbartho]

(Reminder to self): Once I get feedback from users, or in a month or so if I don't get feedback, then I'll merge & deploy the full release.

[jerelmiller]

@fbartho by the way, I did get admin rights to this repo so please let me know if you need anything else changed and I'd be happy to help 🙂

[soulfresh]

I'm experiencing what appears to be module resolution issues when using import { RestLink } from 'apollo-link-rest' with .rc.2 in a webpack-based build. I'm not entirely sure whether this is a packaging issue with the library or something specific to our webpack configuration, but wanted to report what I'm seeing since ES module changes were suggested in this thread.

Environment

• Package version: apollo-link-rest@0.10.0-rc.2
• Bundler: Webpack 5 (via @expo/webpack-config)
• Project: React Native Web application using Expo

Issue 1: Import resolution warning/error

When importing RestLink using the standard import:

import { RestLink } from 'apollo-link-rest'

I get a webpack compilation warning:

  WARNING in ./src/services/report-api/ReportAPI.ts
  export 'RestLink' (imported as 'RestLink') was not found in 'apollo-link-rest'
  (module has no exports)

This shows up as a warning during compilation but causes runtime errors when apollo-link-rest is imported. It's unclear whether this is due to how the ES module re-exports are structured in index.js, or if it's related to webpack's tree-shaking behavior with the "type": "module" package.json setting.

Workaround:
Importing directly from the submodule works:

  import { RestLink } from 'apollo-link-rest/restLink'

Issue 2: Source map warnings

When using the direct import workaround above, we get source map warnings:

  WARNING in ../node_modules/apollo-link-rest/restLink.js
  Module Warning (from ../node_modules/source-map-loader/dist/cjs.js):
  Failed to parse source map from '/Users/xxx/node_modules/src/restLink.ts' file: Error: ENOENT: no such file 
  or directory

The source maps appear to reference source files at incorrect paths (missing apollo-link-rest/ in the path). The source TypeScript files are not included in the published npm package, so these source maps can't be resolved.

Workaround:
I had to suppress these warnings in our webpack config:

  config.ignoreWarnings = [
    {
      module: /node_modules\/apollo-link-rest/,
      message: /Failed to parse source map/,
    },
  ]

Relevant webpack configuration

Our webpack setup is fairly standard, using Expo's webpack configuration with the following relevant settings:

Module resolution (from Expo):

  resolve: {
    mainFields: ["browser", "module", "main"],
    extensions: [".web.ts", ".web.tsx", ".web.mjs", ".web.js", ".web.jsx",
                 ".ts", ".tsx", ".mjs", ".js", ".jsx", ".json", ".wasm"]
  }

Additional config (our customizations):

  // Add mjs file handling which is required by @graphql-tools
  config.module.rules.push({
    test: /\.mjs$/,
    include: /node_modules/,
    type: 'javascript/auto',
  })

  // Source map warning suppression (workaround)
  config.ignoreWarnings = [
    {
      module: /node_modules\/apollo-link-rest/,
      message: /Failed to parse source map/,
    },
  ]

[fbartho]

@soulfresh Thanks for reporting back! This feels like a blocking issue for stabilizing this release.

I unfortunately don't have time to personally try to solve this problem.
I do however have time & I'd be happy to review any PR that fixes this issue targeting this branch, so if you or someone else can help fix the ESModule / build / packaging issues, I'd certainly be happy.

[soulfresh]

I'll give it a try 👍

[jerelmiller]

Got a PR up #339 that addresses some remaining items and adds an exports field to package.json. @soulfresh I'd be curious if this addresses the issue you're seeing or not.

[soulfresh]

@jerelmiller I gave this a quick try and I wasn't able to get it working with your branch. However, that could be on me. Here's what I've tried:

Yarn install git branch

• yarn add apollo-link-rest@https://github.com/apollographql/apollo-link-rest.git
• update apollo-link-rest version to https://github.com/apollographql/apollo-link-rest.git
• yarn install

Yarn link

• (in apollo-link-rest) yarn build
• (in my project) yarn link ../../apollo-link-rest

With both of the above, I updated my imports from apollo-link-rest/restLink to apollo-link-rest but I get the error "Cannot find module 'apollo-link-rest'" with either approach. Additionally, I can no longer import { RestLink } from 'apollo-link-rest/restLink' which breaks my workaround.

I've tried building the project with yarn install && yarn build:browser but that fails with the following error:

Error: Can't walk dependency graph: Cannot find module '/Users/xxx/apollo-link-rest/lib/bundle.umd.js' from '/Users/xxx/apollo-link-rest/lib/_fake.js'

[fbartho]

@soulfresh thanks for helping test!

As I understand it, this project is definitely configured to require a build step, so I wouldn’t have expected your first attempt (git-based) to have worked unless you added a build-step.

I never intended to support deep-imports to the /restLink / (that’s API surface area that I would consider public-api and need to maintain if we offered it, and I would argue against that)

But your second attempt is something I would have expected to work if I were a consumer of this library.

I have nothing further constructive to add here, but I wanted to demonstrate I am paying attention and interested in further research/fixes.

[soulfresh]

Ah ha! I got this working. And yes, I agree that /restLink should not be considered public.

I can confirm that import { RestLink } from 'apollo-link-rest' is now working for me with this branch after running yarn bundle inside apollo-link-rest and then using yarn link ../apollo-link-rest. I would suggest updating the Contributing section of the README to note that yarn bundle is necessary.

Thanks for this fix! 🙏

[fbartho]

I wouldn’t be opposed to a PR that made the yarn bundle command optional/unnecessary when installing via yarn link or git but I don’t know how hard that is / how much maintenance that would entail.

Glad to hear you got it working for your setup! And good call on the CONTRIBUTING guide tweak. Hopefully I or someone will remember to add to that in the future. :P