profusion / apollo-federation-file-upload

Add file upload support to Apollo Federated services.
32 stars 27 forks source link

Feat release 4.1 #55

Closed barbieri closed 1 year ago

barbieri commented 2 years ago

Prepare release 4.1.0 fixing #52 and updating dependencies

bragovo commented 2 years ago

Hello! When you planning to release this? We have a lot of pain about https://github.com/profusion/apollo-federation-file-upload/issues/52 :-D

barbieri commented 2 years ago

Hello! When you planning to release this? We have a lot of pain about #52 :-D

hi @bragovo, just sent a new try based on https://github.com/profusion/apollo-federation-file-upload/issues/52#issuecomment-1148973969, I'm waiting for @glasser to review so I can merge and release.

barbieri commented 2 years ago

@glasser, @bragovo would you mind confirming this works? I'll merge the fix proposed in #57 and release soon (today or tomorrow)

bragovo commented 2 years ago

@barbieri unfortunately my JS teammates still can't upload anything :/ I need some more time to check how to fix it.

barbieri commented 2 years ago

@barbieri unfortunately my JS teammates still can't upload anything :/ I need some more time to check how to fix it.

weird, wasn't the Headers the problem? It should be using the plain Object now. Just check if the #57 pointed issue is not the problem (in other words: if you're relying on headers set at willSendRequest, we need that fix so it works.

rostber commented 2 years ago

That pull make problem. The willSendRequest method will call with undefined headers object (in original RemoteGraphQLDataSource headers always belongs to Headers and never undefined).

By the way, FileUploadDataSource class doesn't send operationName. Way to fix it:

    const operations = JSON.stringify({
      query: request.query,
      variables,
      operationName:
        'incomingRequestContext' in args
          ? args.incomingRequestContext.request.operationName
          : args.request.operationName,
    });
TheBit commented 2 years ago

Hi, any news on this? We can't go live without upload :(

sfounder commented 2 years ago

Hello. Is it so critical to merge? I see few developers are blocked this MR. We also can't go live without upload. Could you merge this MR and then refactor code? Thanks!

paulpdaniels commented 2 years ago

Any help that the maintainers need here? I know we all get busy, so anything I can do to help move this along?

glasser commented 2 years ago

Hi, I found a bit of time to look at this but I'm not sure what I'm being asked to review. It looks like you're working around the Fetcher type declaration with an as typecast, which you need because you're trying to put a FormData as the body which Fetcher doesn't expect. But as I said I don't think that arbitrary Fetch implementations all handle FormData in the same way. So doing it this way is essentially locking your API in to only working with Fetch implementations that happen to work with the FormData implementation you chose. To me this seems likely to lead to confusion. My recommendation continues to be that if this package depends on the ability to send FormData (as implemented by the form-data npm package) via a Fetch API, then it should just choose a particular implementation of Fetch that definitely supports the form-data package and use it directly instead of allowing the fetcher to be overridden with an arbitrary implementation that might not work.

glasser commented 2 years ago

(That said, if you're OK with the idea that your package will be fragile and could break if a user passes in a Fetcher implementation that doesn't support FormData from form-data as a body, then this is probably fine? I'm not an expert on the details of your project but if it works and passes tests then it's probably OK?)

TheBit commented 1 year ago

@cabelitos sorry for disturbing, but this is waiting for half a year, are there any plans for this?

BobbiSixkiller commented 1 year ago

Is this going to be released any time soon please ?

oliveirarleo commented 1 year ago

Hello @TheBit and @BobbiSixkiller. We cannot expect help from @cabelitos anymore, but our team at ProFUSION will put some hands here to help you guys out right at our next sprint. We now have officially including this task in our planning. That means we'll start working (and revise the work done here) as of 30th of January and we expect to solve the problem and merge between 2 and 4 weeks.

oliveirarleo commented 1 year ago

:tada: