Skip to content

Conversation

LitoMore
Copy link
Contributor

@LitoMore LitoMore commented Aug 10, 2025

I used the package.json overrides to override the version of http-deceiver. Here is the related solution: spdy-http2/http-deceiver#7 (comment). However, I avoided using the npm package http-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 a package-lock.json related issue.

Some unrelated changes are made by npx prettier --write ..

@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2025

Messages
📖 ✨ Thanks for your contribution to Shields, @LitoMore!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 41a3914

@LitoMore LitoMore force-pushed the unblock-http-parser branch 6 times, most recently from f80e15a to 18099d4 Compare August 12, 2025 11:53
@LitoMore LitoMore force-pushed the unblock-http-parser branch from 18099d4 to 41a3914 Compare August 12, 2025 11:59
@LitoMore LitoMore marked this pull request as ready for review August 23, 2025 10:44
Copy link
Member

@jNullj jNullj left a 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.

@LitoMore
Copy link
Contributor Author

Thank you for sharing your thoughts.

I already credit both the original author and the fork author at the header of the vendor/http-deceiver/index.js file.

But let me close this since you don't like this idea.

@LitoMore LitoMore closed this Aug 24, 2025
@LitoMore LitoMore deleted the unblock-http-parser branch August 24, 2025 04:01
@jNullj
Copy link
Member

jNullj commented Aug 24, 2025

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.
"not liking it" does not mean it's not a good option, just that my intuition is to not add it. But that's not a clear or professional language, and does not help build a good discussion so I should have avoided it.

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.

@LitoMore
Copy link
Contributor Author

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.

@LitoMore LitoMore restored the unblock-http-parser branch August 25, 2025 05:10
@LitoMore LitoMore reopened this Aug 25, 2025
@jNullj
Copy link
Member

jNullj commented Aug 30, 2025

I have some ideas to reduce the risks for the override to be overlooked or stay outdated.

  1. We can add a postinstall warning.
  2. We can add a readme with info at the override folder (vendor/http-deceiver/)
  3. Regardless of what we add, I should open an issue until this is resolved one way or another.
  4. This is a bit more complex, but we could add a follow up PR with CI to test for upstream changes of the package.
  5. Maybe just simple a scheduled reminder in CI/calendar.

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?
I don't say we have to do all of these, just brainstorming here.

Regarding the license, if you take a look at the repository You can see the license clearly states

.....subject to the following conditions:

The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.

While I appreciate the effort to credit the original creator and mention the MIT license in the header of vendor/http-deceiver/index.js, which is a good practice, I think we are still required to include the full license notice as mentioned in the original repository, even when the software license is permissive.

@PyvesB Would love if you could give us your two cents, i have little experience with maintaining overrides over time.

@PyvesB
Copy link
Member

PyvesB commented Sep 1, 2025

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.:

  • We can add a readme with info at the override folder (vendor/http-deceiver/)

  • Regardless of what we add, I should open an issue until this is resolved one way or another.

  • include the full license notice as mentioned in the original repository, even when the software license is permissive

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. :)

@paulmelnikow
Copy link
Member

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 overrides, by having Scoutcamp directly import the replacement module.

@PyvesB
Copy link
Member

PyvesB commented Sep 27, 2025

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? :)

@chris48s
Copy link
Member

Had a quick look for you.
I think Paul is the only person who can add a new maintainer to @shields_io/camp on npm

Screenshot at 2025-09-27 11-16-08

@paulmelnikow
Copy link
Member

Seems like the permissions are something we ought to adjust regardless.

@PyvesB PyvesB added the core Server, BaseService, GitHub auth, Shared helpers label Oct 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Server, BaseService, GitHub auth, Shared helpers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants