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

mscdex Updated his projects #78

Closed
2 tasks done
Uzlopak opened this issue Dec 20, 2021 · 9 comments
Closed
2 tasks done

mscdex Updated his projects #78

Uzlopak opened this issue Dec 20, 2021 · 9 comments

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Dec 20, 2021

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

mscdex updated his packages. I did a benchmark and our fork is slightly faster than his busboy. Maybe we should take a look at his code changes and check if we can apply those changes to our fork.

@shakaran
Copy link

@Uzlopak if we gonna update in mongo-express repository and trust in this fork over the original source. This should keep in sync the new changes. I see PR with more than 6 months too, so this should be more updated, and hopefully get 0 PR and 0 issues

@Eomm
Copy link
Member

Eomm commented Sep 3, 2022

Here is the changelog: mscdex/busboy#266

@fastify/forked-projects

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Sep 3, 2022

@shakaran

I dont have currently the time nor the motivation to put work in this package. I am currently working at fastify-session, which is quite complex.

Our fork works properly and has closed all the security vuln. for sure. You updated mongo-express to busboy 1.6.0, so there is no need to switch to our fork. Our offer to switch to our fork was at a time that mscdex was not patching the sec. vuln.. If you decide to use busboy 1.6.0 than it is your good decision and I support that.

@kibertoad
Copy link
Member

I second what uzlopak has said. I'm happy to review any community submissions, but I don't have much time to put into active evolution of this package. upstream busboy is a good choice these days, if it works for you, staying with it makes sense. We have some smaller improvements and features that upstream doesn't have, but you not necessarily would benefit from them.

@gurgunday
Copy link
Member

The performance difference is still significant on my machine, maybe it'd be better to cherry pick commits

| Node    | Option         | Msecs/op       | Ops/sec  | V8                    |
| ------- | -------------- | -------------- | -------- | --------------------- |
| 20.5.0  | fastify-busboy | 0.110761 msecs | 9028.472 | V8 11.3.244.8-node.10 |
| 20.5.0  | busboy         | 0.210623 msecs | 4747.830 | V8 11.3.244.8-node.10 |
| 16.13.0 | fastify-busboy | 0.270984 msecs | 3690.248 | V8 9.4.146.19-node.13 |
| 16.13.0 | busboy         | 0.340114 msecs | 2940.190 | V8 9.4.146.19-node.13 |

**Specs**: M1 (2.4 GHz)

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Aug 19, 2023

If there are optimizations we can utilize, why not

@mcollina mcollina closed this as completed Oct 4, 2023
@rtritto
Copy link

rtritto commented Oct 22, 2023

This forked branch is 23 commits behind, please reopen this issue.

FYI @Uzlopak @mcollina

@gurgunday
Copy link
Member

gurgunday commented Oct 22, 2023

We actually wanted to remove the fork reference because we've started to diverge from the other implementation in important ways that moved us closer to the RFC and gave us performance improvements

So not all the commits can be applied to ours anymore

#130

@rtritto
Copy link

rtritto commented Oct 22, 2023

I think is needed a new issue/discussion to track missing commits and their state (to do, merged or rejected).

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

No branches or pull requests

7 participants