-
Notifications
You must be signed in to change notification settings - Fork 12
test: remove node-static dependency #56
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
Conversation
tamird
commented
Dec 2, 2025
- test: avoid relying on cwd
- test: remove node-static dependency
|
@HighCommander4 please see individual commits for an easier review. |
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.
Pull request overview
This PR removes the node-static dependency from the test suite by implementing a custom static file server using Node's built-in http and fs modules. The changes also improve path handling by using path.resolve and path.join instead of relying on process.cwd(), making the tests more robust to execution context.
- Replaces
node-staticwith a custom HTTP server implementation for serving test assets - Updates path construction to use
path.resolveandpath.joinfor better cross-platform compatibility - Removes
node-staticand@types/node-staticfrom package dependencies
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/index.ts | Replaces node-static with custom file server implementation and updates asset paths to use path module methods |
| package.json | Removes node-static and @types/node-static from devDependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1b7942d to
bc9c8be
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.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bc9c8be to
4fa84cd
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.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7711115 to
4ddda02
Compare
|
(I think you meant to set me as reviewer rather than assignee.) I've added this to my review queue, but just as a heads up, my queue of clangd-related reviews is very long at this point, and it may be quite a while before I get to this. I've added some other reviewers as well (@hokein, @kadircet, @sam-mccall) and I would really appreciate some help with clangd reviews. |
|
Ack, thanks. As an aside, I requested codex access to clangd repositories, which among other things allows me to use |
I can't see the request. I think it requires admin permission which I don't have. |
|
Well, this is removing a test-only dependency, I'll go ahead and merge it; probably on Monday. |
This library appears to be unmaintained and has transitive dependencies that have security advisories, which in turn trigger security advisories in https://github.com/clangd/node-clangd/security/dependabot. Thus replace it with a tiny HTTP handler and remove the bad dependencies.
4ddda02 to
446becc
Compare