Adds pbf format support to Node.js bindings #5794
Conversation
src/bindings/nodejs/src/valhalla.cc
Outdated
| return worker->GetPromise(); | ||
| } | ||
|
|
||
| Napi::Value CreateAsyncRequestWithFormat(const Napi::CallbackInfo& info, |
There was a problem hiding this comment.
I don't understand the need for this function: in the ValhallaAsyncWorker you already have the request available, so you can do the format checking in OnOK() directly?
I also wonder if we couldnt do that in the JS layer where we know the format already (prior to calling C++ Actor) and save ourselves the string -> ptree conversion in the c++ layer. wdyt @roadrouting @SiarheiFedartsou ?
There was a problem hiding this comment.
Yeah, I was looking for a way to avoid double-parsing the request, but didn't see an easy way on the c++ side. Doing it in the JS layer seems better, so I have done that.
roadrouting
force-pushed
the
5742-javascript-binary-formats
branch
from
e855e36 to
f091b32
Compare
|
@nilsnolde thanks for the quick review! Changes made.
Correct, this is only for the output |
nilsnolde
left a comment
nilsnolde
left a comment
There was a problem hiding this comment.
thanks for addressing the review!
just a small confusion left what osmc is in the format context, which can probably go.
|
oops, theres a problem with packaging: https://github.com/valhalla/valhalla/actions/runs/20619227708/job/59244569602?pr=5794 |
The package_nodejs_bindings job was failing because the proto directory wasn't included in the sparse-checkout, causing the create-package.sh script to fail when trying to copy proto files for npm distribution.
|
Thanks! Changes pushed and that should hopefully fix the CI |
Add darwin/, linux/, and valhalla_* to package.json files array. When the files array was introduced in commit 08aee166d, it omitted the platform-specific binary directories, causing the native bindings to be excluded from the published package.
|
@nilsnolde fix pushed for the latest CI failure |
2 participants
The problem was described in #5742 that the Node.js bindings did not support binary outputs, and they would result in corrupt strings.
This PR adds support for the 6 methods I found that support returning a PBF format.
While I was at it, I:
matrix,isochrone,expansion) and added Node.js bindings section to protocol-buffers.md