potiuk commented on PR #55:
URL: https://github.com/apache/comdev/pull/55#issuecomment-6271157822
Thanks — the proxy docs and warning are a nice usability fix. A couple of
things before merge:
1. **Lock files**: `ponymail-mcp/package-lock.json` still records
`"engines": {"node": ">=20"}`, and `apache-projects-mcp/package-lock.json`
doesn't have the new `engines` entry. Could you run `npm install
--package-lock-only` in both and commit the result? #56 adds a prek hook
(`package-lock-sync`) that catches exactly this, so once it lands this kind of
drift fails at commit time.
2. **CI matrix**: `["22"]` resolves to latest 22.x, so neither the declared
floor nor Node 24 LTS is exercised. Suggest `node-version: ["22.21", "24"]`.
Smaller points on the warning, take or leave:
- `!process.env.NODE_USE_ENV_PROXY` treats `NODE_USE_ENV_PROXY=0` as
enabled, and doesn't account for `--use-env-proxy` passed via CLI /
`NODE_OPTIONS` (which would trigger a false warning). Checking `=== "1"` plus
`process.execArgv` / `NODE_OPTIONS` would cover both.
- On Node < 22.21 the env var is silently ignored and no warning fires;
`engines` is advisory only, so a version check might be worth adding.
- In ponymail-mcp the check runs at module top level, while
apache-projects-mcp has it inside the main guard. Might be worth aligning.
- Nit: the snippet in `apache-projects-mcp/README.md` is missing the `json`
fence tag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
