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
implement sync formdata parser #2911
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2911 +/- ##
==========================================
- Coverage 93.86% 93.62% -0.25%
==========================================
Files 85 86 +1
Lines 23418 23861 +443
==========================================
+ Hits 21981 22339 +358
- Misses 1437 1522 +85 ☔ View full report in Codecov by Sentry. |
Alternatively we could just take Deno's parser. |
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
Does this increase our wpt conpliance? |
Yes, there are 4 (out of 14 tests wrt .formData) that now pass and there are some untested behaviors/behaviors we had to workaround that are now correct:
// Wait a tick before checking if the request has been aborted.
// Otherwise, a TypeError can be thrown when an AbortError should.
await Promise.resolve() |
This comment was marked as off-topic.
This comment was marked as off-topic.
@Uzlopak ptal |
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
We can either land this as-is or I can spend the next few days/week (its midterm week for me so it might take longer) porting the busboy tests and fixing edge cases. |
how about a new branch and port the tests, and then when they are ported we merge it into main? Can you port a test from busboy so that i can assess how much work it is? |
Adding the tests is very simple for the sole reason that they don't use any test runner and rely completely on node:assert. I ported the tests in my old formdata parser branch: https://github.com/KhafraDev/undici/tree/undici-formdata-parser/test/busboy However, I don't think there's much benefit in porting something without also fixing bugs. |
It might be a little harder, given that we don't implement any limits, nor does the parser extend EventEmitter much less emit events, but it shouldn't take more than an hour? |
ah you mean the original busboy. |
I assumed that @fastify/busboy would have the same tests, guess I was wrong. Busboy's tests seem more relevant here since they test the actual parsing/output, where as fastify/busboy seems to test more so the actual api (which obviously we'd fail). |
actually busboy (original) mixed dicer and co into it. @fastify/busboy vendored them. Anyhow i think that our @fastify/busboy tests were kind of more reasonable. What about these tests? https://github.com/fastify/busboy/blob/master/test/types-multipart.test.js |
No preference, but I don't like how |
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.
looks good to me
Fixes #2890
Based on https://andreubotella.github.io/multipart-form-data/