-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
chore: unblock the deprecated http_parser
issue
#11279
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
base: master
Are you sure you want to change the base?
Conversation
|
f80e15a
to
18099d4
Compare
18099d4
to
41a3914
Compare
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 like the idea of an override but this block is in both camp which we did not change to express for some time now and docusaurus which we don't plan changing soon.
If im not wrong we are required to add a license somewhere, this is MIT so i think we need a copy of the license with credit to the original creator.
Thank you for sharing your thoughts. I already credit both the original author and the fork author at the header of the But let me close this since you don't like this idea. |
In retrospective my comment above is hasty and poorly made, It does not present constructive feedback or present clear technical rational. I apologize for the lack of communication skills here. My concern with this solution is maintainability, it's harder to keep track of, and can cause issues in the future, my bigger fear is that we might overlook this when updating dependencies as it may be forgotten. You are right that this might serve to solve the issue of updating to node 24, and I'm thankful for you picking up such idea I was not aware of before. I tried to convey this fix might be needed as we have dependencies that require this with no clear path to change them soon. But after reading my comment I can not really expect anyone to understand it from the way it's written. If you're still interested in this, please feel free to re-open the pull request. I understand if my previous feedback has made you hesitant to work with me on this, so please know you're also welcome to request a review from another maintainer. I'll do better at communicating my thoughts in the future. |
Really appreciate you taking the time to look back at the PR and write such a thoughtful follow-up. It’s totally normal that a PR gets rejected—that’s part of the process—and your concerns about maintainability and potential issues during dependency updates are valid. Your clarification helps a lot, and I’m grateful for the thoughts around Node 24 and the dependency constraints. No hard feelings at all about the earlier comment—I value the honesty and the effort you put into refining the feedback. I’m still interested. I can revisit the approach with maintainability in mind (e.g., clearer docs, guardrails, or a scoped implementation) and reopen the PR. I’m also fine looping in another maintainer for a second perspective if helpful. Thanks again for the thoughtful follow-up—I appreciate it. |
I have some ideas to reduce the risks for the override to be overlooked or stay outdated.
Any CI could be generic to overrides and doesn't have to be specific to this PR, but a nice to have. What do you think of the suggestions, are there any new ideas or improvements? Regarding the license, if you take a look at the repository You can see the license clearly states
While I appreciate the effort to credit the original creator and mention the MIT license in the header of @PyvesB Would love if you could give us your two cents, i have little experience with maintaining overrides over time. |
Thanks for looking into this, and for the thoughtful interactions! I don't recall us having to maintain an override like this on the Shields.io, even though I've had to do this in personal projects (example here). My take is that it's a package with a single not-too-long file, so I don't think we'd be causing ourselves huge headaches down the line by doing this version override. Of course, in an ideal world, we'd want to move away from Scoutcamp and stop depending on http-deceiver, but I'm well aware that's a larger undertaking. I'd be happy for us to move forward with this, by adding some of the things @jNullj suggested, i.e.:
Adding warnings or CI checks sound like good ideas, but possibly over-engineered for the present case. Let's keep things as simple as possible. :) |
Indeed, moving away from Scoutcamp is a larger undertaking. If this is a only needed for Scoutcamp, I suggest we fix it in our Scoutcamp fork instead as Chris suggested in #11071. Perhaps also we could avoid the need for using |
Fixing it in our Scoutcamp fork would indeed be cleaner, though it would be a fair bit more effort (respinning another contribution, publishing a new NPM package revision, bumping the dependency here). Worth nothing that only @paulmelnikow and @chris48s presently have ownership over the package, we'd need to get permissions fixed as well. @LitoMore as the contributor here, what are your thoughts? :) |
Seems like the permissions are something we ought to adjust regardless. |
I used the package.json
overrides
to override the version ofhttp-deceiver
. Here is the related solution: spdy-http2/http-deceiver#7 (comment). However, I avoided using the npm packagehttp-deceiver-fixes
directly due to the potential supply chain attack issue.This approach unblocked the deprecated
http_parser
issue for #11071 so that we can move forward with Node.js 24 support.I will take a look at the failing CI. It should be apackage-lock.json
related issue.Some unrelated changes are made by
npx prettier --write .
.