-
Notifications
You must be signed in to change notification settings - Fork 563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for multipart/form-data #1606
Conversation
Exciting! |
Codecov Report
@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
- Coverage 95.21% 94.57% -0.65%
==========================================
Files 50 53 +3
Lines 4748 4828 +80
==========================================
+ Hits 4521 4566 +45
- Misses 227 262 +35
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
busbuy will emit an 'error'
event: https://github.com/mscdex/busboy/blob/9aadb7afbcb8c70c81c93b1018313c1b1835afb0/lib/types/multipart.js#L398.
This also needs tests for the error path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failure
lib/fetch/body.js
Outdated
}) | ||
}) | ||
busboy.on('error', (err) => { | ||
throw Object.assign(new TypeError(), { cause: err }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just throw here... this will crash the process. Is there tests for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just throw here... this will crash the process.
Do you have a suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to reject this promise https://github.com/nodejs/undici/pull/1606/files#diff-bd6db9a8bceb4e9fce3f93ba5284c81f46a44a1187909e8609006e1efeab2570R435
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@mcollina I think @KhafraDev is working on a multipart parser without any deps. Should we maybe wait and see how that go before landing this? |
@ronag Do we expect new custom solution to work faster/smoother than a solution that was used across entire Node ecosystem for years? What would be the criteria for comparison after it is implemented? |
I would prefer we use busboy at this stage. |
Thanks for the help @mrbbot !
Yes, this was my sentiment from my comment in #974.
At minimum it would need to pass all of busboy's tests (similar to what we do with node-fetch tests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work, there are a couple of comments to fix and we'd be good for landing.
const base64 = encoding.toLowerCase() === 'base64' | ||
const chunks = [] | ||
value.on('data', (chunk) => { | ||
if (base64) chunk = Buffer.from(chunk.toString(), 'base64') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just decrypt a base64 chunk, because the length of the base64 chunk must be a multiple of 4 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const data = Buffer.from(Buffer.from('hello world').toString('base64'));
const chunks1 = [];
const chunks2 = [];
for (let offset = 0, step = 6; offset < data.length; offset += step) {
const chunk = data.subarray(offset, offset + step);
// Decode each chunk
chunks1.push(Buffer.from(chunk.toString(), 'base64'));
// Here we collect as is
chunks2.push(chunk);
}
// Decode after the transfer is completed
const buffer = Buffer.from(Buffer.concat(chunks2).toString(), 'base64');
// hell�v�ld
console.log(await new Blob(chunks1).text());
// hello world
console.log(await new Blob([buffer]).text());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the https://nodejs.org/api/string_decoder.html for this purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina does string decoder need to be applied somewhere in this code explicitly, or it will be called down the line automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameron-robey see what @repsac-by is suggesting, plus test to cover that case. inlining the code should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem exactly? You need 4 bytes to decode base64 at minimum, so why don't you accumulate until you have 4 bytes of multiples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina I'm assuming that you mean: push chunks until the total length of unprocessed data is divisible by 4, then decode and clear the chunk array, and repeat with the next pieces of data. Accumulating like that seems unreliable, since it cannot be known when that multiple of 4 will be reached. It might happen only after a huge number of chunks. The streaming method suggested above is superior because the number of unprocessed bytes that are kept in memory never exceeds 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one in the comment also looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cameron-robey Do you need any help with the remaining part?
Will get to finishing the last couple of things by the end of the week 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is mostly good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@ronag why did we merge this without addressing all comments? should I create a follow-up PR? |
@kibertoad Please open a follow-up PR. |
Is there somewhere we can track the inclusion of this update into Node? |
* add support for multipart/form-data * Handle busboy errors * linting * Catch emitted error * reject promise instead of throwing error * Add test for base64 encoded multipart/form-data Thanks for the help @mrbbot ! * Move busboy from devDependencies to dependencies * Add test for busboy emitting error * Rewrite tests * Update tests to avoid promises and callbacks
* add support for multipart/form-data * Handle busboy errors * linting * Catch emitted error * reject promise instead of throwing error * Add test for base64 encoded multipart/form-data Thanks for the help @mrbbot ! * Move busboy from devDependencies to dependencies * Add test for busboy emitting error * Rewrite tests * Update tests to avoid promises and callbacks
Closes #974