bytewitchcraft / apollo-absinthe-upload-link

A network interface for Apollo that enables file-uploading to Absinthe back ends.
61 stars 37 forks source link

Use native `fetch` by default / make rxjs optional #18

Closed jgonera closed 4 years ago

jgonera commented 5 years ago

The readme says:

You can use the fetch option when creating an apollo-absinthe-upload-link to do a lot of custom networking.

Which made me think that if I don't use custom fetch, then browser's native fetch would be used. That is not the case and instead ajax from rxjs is being used if custom fetch is not provided. This has two major problems:

  1. Observable returned when rxjs is used does not have the same interface as a regular Observable (I am not sure how this is happening, but see the implications below).
  2. apollo-absinthe-upload-link's size is over 50KB gzipped, over 48KB being rxjs.

When it comes to number 1, composing with other links can be problematic. Let's say we have another link like this:

const logTimeLink = new ApolloLink((operation, forward) => {
  return forward(operation).map((data) => {
    // data from a previous link
    const time = new Date() - operation.getContext().start;
    console.log(`operation ${operation.operationName} took ${time} to complete`);
    return data;
  })
});

(https://www.apollographql.com/docs/link/overview.html)

Then if we try to use it together with apollo-absinthe-upload-link like this:

ApolloLink.from([logTimeLink, apolloAbsintheUploadLink])

we will get the following error:

Error: Network error: forward(...).map is not a function

Since the app I'm working on is compatible only with modern browsers, I work around this by initializing the link like this:

const apolloAbsintheUploadLink = createLink({ fetch, uri: '/graphql' });

This skips the whole rxjs code path, which brings me to point number 2. I am including 48KB of dead code:

screen shot 2018-12-05 at 4 04 32 pm

My proposal is to by default use browser's native fetch and remove rxjs as a dependency. It can be mentioned in the readme that if someone needs compatibility with older browsers, they can provide a custom fetch function.

jgonera commented 5 years ago

I'm happy to submit a PR if that sounds good, but I would like to know whether I'm missing something before I do that.

ihorkatkov commented 5 years ago

Hi @jgonera. Thanks for the info. I think you have interesting points and I need to think about it.

ihorkatkov commented 5 years ago

@jgonera can you submit a PR, please?

tcitworld commented 3 years ago

@ihorkatkov May I ask why this is closed? It doesn't seem like this is fixed.

ihorkatkov commented 3 years ago

@tcitworld Hey! No, it wasn't handled. I would be happy if you could drop rxjs altogether. Thanks

luin commented 1 year ago

Hi @ihorkatkov 👋

Is there still any interest in ditching rxjs? fetch is now well-supported by all modern browsers so I'm thinking that we can remove rxjs and release a new major version. People who need polyfill can provide their own.

I can create a pull request if it makes sense.