Skip to content

Conversation

@afarber
Copy link
Contributor

@afarber afarber commented Nov 20, 2025

Fix #7272 by:

  • Update BINDING_DIR from lib/binding to lib/binding_napi_v8 to align with node-pre-gyp's versioning
    conventions
  • Add file(MAKE_DIRECTORY) to create the binding directory during CMake configuration
  • Update all references in lib/index.js, package.json, CI scripts, and documentation

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@afarber
Copy link
Contributor Author

afarber commented Nov 22, 2025

Hi @DennisOSRM and @SiarheiFedartsou please take a look at my fix

@YoelRidgway
Copy link

Thanks for this @afarber. I wasted so much time trying to understand what was going on!

@afarber afarber force-pushed the 7272-node-binding-path branch from 43baee5 to 05a1f30 Compare January 2, 2026 21:36
@afarber
Copy link
Contributor Author

afarber commented Jan 2, 2026

Hi @DennisOSRM please consider merging this fix and then releasing the 6.0.1 version

Copy link
Contributor

Copilot AI left a 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 fixes the Node.js binding path to align with node-pre-gyp versioning conventions by updating the binding directory from lib/binding to lib/binding_napi_v8.

Key changes:

  • Updated BINDING_DIR in CMakeLists.txt to use lib/binding_napi_v8 and added directory creation during CMake configuration
  • Removed the lib/binding_napi_v8 symlink that previously pointed to lib/binding
  • Updated all references across code, documentation, CI scripts, and configuration files to use the new path

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/nodejs/CMakeLists.txt Updates BINDING_DIR variable to new path and adds file(MAKE_DIRECTORY) command to ensure directory exists
lib/index.js Updates require path to load native binding from new location
src/nodejs/node_osrm.cpp Updates documentation comments with corrected binary paths
docs/nodejs/api.md Updates API documentation examples with new binding path
package.json Updates nodejs-tests script to reference new binary location
scripts/ci/run_benchmarks.sh Updates benchmark script to use new node_osrm.node path
scripts/ci/node_package.sh Updates packaging script to check binary in new location
.github/workflows/osrm-backend.yml Updates commented-out test commands with new binary paths
.gitignore Updates ignored directory from lib/binding to lib/binding_napi_v8
lib/binding_napi_v8 Removes symlink that previously pointed to binding directory
CHANGELOG.md Adds entry documenting the fix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Collaborator

@DennisOSRM DennisOSRM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good sleuthing finding this fix. 😉

@DennisOSRM DennisOSRM merged commit 27b3ea1 into Project-OSRM:master Jan 3, 2026
27 checks passed
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.

Fix error: Cannot find module './binding/node_osrm.node'

3 participants