Skip to content

#257: replace sync-request with curl#950

Open
nacNAC333 wants to merge 1 commit into
objectionary:masterfrom
nacNAC333:257
Open

#257: replace sync-request with curl#950
nacNAC333 wants to merge 1 commit into
objectionary:masterfrom
nacNAC333:257

Conversation

@nacNAC333

Copy link
Copy Markdown

Fixes #257

Replaces the sync-request npm package with curl via child_process.execSync for fetching Maven Central metadata.

sync-request pulls in a large transitive dependency tree and has known issues with newer Node versions. curl is universally available on all supported platforms (macOS, Linux, Windows via PowerShell).

Changes:

  • Rewrite parser-version.js to use execSync("curl ...") instead of sync-request
  • Remove sync-request from package.json dependencies
  • Add exists() method for version validation using HTTP HEAD check

@yegor256

Copy link
Copy Markdown
Member

@nacNAC333 Thanks for your contribution, we appreciate it! Before we can merge this pull request, we need to see all CI workflows pass without errors. We can't merge the pull request into the master branch unless all workflows are green. Try to fix them, also paying attention to code style issues.

@999axel999

Copy link
Copy Markdown

Thanks for working on this. I noticed a few blockers worth fixing before merge:

  1. package.json removes sync-request, but package-lock.json is not updated. The lockfile still lists the root dependency and the node_modules/sync-request package, so npm ci/CI may fail or continue installing the removed dependency. Please regenerate the lockfile together with the manifest change.

  2. exists() uses curl ... -o /dev/null, but this package declares Windows support via os: ["darwin", "linux", "win32"]. /dev/null is not portable to Windows, so parser version validation can fail on a supported platform. Either keep this code path platform-neutral or avoid shelling out to curl here.

Also, issue #257 explicitly asks for a Maven-native implementation via versions-maven-plugin; this patch still reads Maven Central metadata directly, just through curl, so it may not satisfy the requested design.

@yegor256

yegor256 commented Jun 8, 2026

Copy link
Copy Markdown
Member

@nacNAC333 merge conflict here

@nacNAC333

Copy link
Copy Markdown
Author

Rebased on master and squashed to one commit. All three review points addressed:

  1. Lockfile — regenerated from scratch, sync-request fully removed from both package.json and package-lock.json.
  2. Windows portability — replaced curl//dev/null with a Node.js subprocess using the built-in https module. No platform-specific paths or binaries.
  3. Maven-native — fair point. This PR removes the deprecated sync-request dependency (which has known security issues and is unmaintained) using only Node.js built-ins. A versions-maven-plugin approach would be a separate, larger change to the build system. This is a strictly narrower improvement: same HTTP calls, no new dependencies, cross-platform.

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

Successfully merging this pull request may close these issues.

--latest implementation is not Maven-native

3 participants