Skip to content
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

Update dependencies #1

Closed
wants to merge 3 commits into from
Closed

Update dependencies #1

wants to merge 3 commits into from

Conversation

alexohneander
Copy link

  • update busboy, dale and mime
  • add standard nodejs gitignore
  • add new initialization of busboy

@fpereiro
Copy link
Owner

Hi @alexohneander, thank you for your contribution!

Reviewing the PR, I notice the following changes concerning Busboy:

  • Update Busboy from 0.3.1 to 1.6.0
  • Remove the new operator when initializing Busboy. This is indeed necessary to upgrade to Busboy v0.
  • Remove the try/catch block in the initialization of Busboy.

Updating Busboy is indeed necessary and long overdue. However, I'm afraid that these changes by themselves are not enough. The list of breaking changes from moving from Busboy v0 to v1 is here (mscdex/busboy#266) and I believe that at least a few more changes need to be implemented.

I don't think that removing the try/catch block is an improvement, since I've run into issues where invalid headers will make Busboy throw an error. I believe the right behavior is to catch it and pass it to a callback.

As for the rest of the changes, I am afraid I don't see the value in them:

  • The projects in the ustack (https://github.com/fpereiro/ustack) don't have a .gitignore, to keep the file count as low as possible. I understand this is unconventional. However, it is part of the project's rabid minimalism.
  • The code has been thoroughly (and probably automatically) reformatted and made to use ES6. This is undesirable for the following reasons: 1) ll the libraries in the ustack use mES5 (https://github.com/fpereiro/ustack#mes5-a-javascript-subset), a subset of ES5 Javascript. 2) the existing code is formatted in a consistent way that I personally appreciate; it's definitely not an industry standard, but it has a consistent style across the ustack.

For those reasons, I have to close the PR without merging it.

If you are interested in helping me figure out what precise changes are needed to make cicek work with Busboy v1, that will be very welcome. Alternatively, if you're interested in using cicek when it uses a newer version of Busboy, i can mention you when I implement those changes myself.

Once again, I appreciate your contribution and am again sorry I cannot merge it.

Cheers!

@fpereiro fpereiro closed this Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants