feat: multipart/form-data support #543
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f8e1957:
|
tkamenoko
force-pushed
the
feature/graphql-multipart
branch
from
0b3d593
to
21664f2
Compare
test/graphql-api/multipart.mocks.ts
Outdated
| >('UploadFile', async (req, res, ctx) => { | ||
| const { file1, file2, files, plainText } = req.variables | ||
| const filesResponse: string[] = [] | ||
| for (const file of files ?? []) { |
There was a problem hiding this comment.
?? allows files to equal a falsy value (i.e. false). This would still break the for/of loop:
for (const file of false) // false is not iterableI suggest we use || instead. Nullish coalescing operator (??) is useful when dealing with possibly falsy values, which isn't the case for files. If files is undefined or falsy we should always fallback to an empty array to not break the for/of loop.
test/graphql-api/multipart.mocks.ts
Outdated
| } | ||
| >('UploadFile', async (req, res, ctx) => { | ||
| const { file1, file2, files, plainText } = req.variables | ||
| const filesResponse: string[] = [] |
There was a problem hiding this comment.
What do you think if we use Promise.all and a plain files.map instead of the for/of loop?
const { files = [] } = req.variables
const filesResponse = await Promise.all(files.map(file => file.text())It's going to be less code, making this example more focused. Promise.all would also be a more recommended approach so that you don't wait for each file to be processed individually, allowing parallel transformations to text.
I'm curious to hear your opinion.
|
|
||
| const isMultipart = Boolean(map && fileContents) | ||
| const headers = isMultipart ? {} : { 'Content-Type': 'application/json' } | ||
| let body: string | FormData |
There was a problem hiding this comment.
I'd refactor this part as follows:
const body = isMultipartData ? getMultipartGraphQLBody(operations, ...) : operationsThis will make executeOperations much easier to read by delegating the logic that creates a FormData into a separate utility function that we can also test separately.
There was a problem hiding this comment.
Hey, @tkamenoko. Thank you for your hard work on this! It looks fantastic.
I've left a few review comments, could you please take a look? My main suggestions were:
- Don't move
runtimedeclarations to abeforeAll/afterAllhooks. I see this as an additional internal refactoring. Let's keep the changes of this pull request scoped. - A few naming and structuring suggestions.
I'm excited to see this landing in the next release. Once more, huge thanks!
tkamenoko
force-pushed
the
feature/graphql-multipart
branch
from
dbc53d5
to
da4e1de
Compare
There was a problem hiding this comment.
This looks solid. Let me know if you think these changes are ready to be merged. I'll give them a test locally to get to know multipart data scenarios better.
Thank you for your work on this, @tkamenoko!
|
@kettanaito Thanks for reviewing! I think this PR is ready to merge. Please do it whenever you want. |
kettanaito
force-pushed
the
feature/graphql-multipart
branch
from
60ee756
to
2c6a491
Compare
There was a problem hiding this comment.
This is an incredible addition. Thank you, @tkamenoko! Welcome to contributors 🎉
|
Wow, great addition. Thanks for the contribution. |
This PR fixes #215 .
It contains multipart/form-data parser that enable to parse a request body into an object.
It also has variables builder based on this spec .
In addition, rest-api can handles multipart/form-data now.