-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Expose busboy options, update docs and tests. #34
Conversation
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.
LGTM
index.js
Outdated
@@ -40,7 +40,7 @@ function fastifyMultipart (fastify, options, done) { | |||
|
|||
const req = this.req | |||
|
|||
const stream = new Busboy({ headers: req.headers }) | |||
const stream = new Busboy(Object.assign({ headers: req.headers }, options)) |
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.
@nigelhanlon I would prefer if we copied the options using an Object.keys()
and for
loop instead. Object.assign()
is super slow and it will likely still be slow in Node 10 (see https://twitter.com/dan_abramov/status/980436488860196864).
index.js
Outdated
const busboyOptions = { headers: req.headers } | ||
for (let opt of Object.keys(options)) { | ||
busboyOptions[opt] = options[opt] | ||
} |
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.
for of
is super slow as well (also using let
in loops is slow), you'd need a nice old for (var i = 0; i< n; i++)
.
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.
Do you mean something like this?
const busboyOptions = { headers: req.headers }
const keys = Object.keys(options)
for (var i = 0; i < keys.length; i++) {
busboyOptions[keys[i]] = options[keys[i]]
}
const stream = new Busboy(busboyOptions)
Can you remove the spurious package-lock file and add it to .gitignore? |
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
This PR allows end users to pass options to the internal busboy module when registering the plugin with fastify. This is useful as you currently cannot set upload limits which are supported by busboy internally.
The supported options are documented on the busboy page here. I have included an updated readme and updated tests to reflect the proposed changes. Feed back and suggestions welcome.