search

RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
319 stars 70 forks source link

support node 14 #723

Closed koonpeng closed 2 years ago

koonpeng commented 3 years ago

This is an ongoing work to support node >= v14.

node v14's version of v8 has changed the API for ArrayBuffer::New which cause rclnodejs to stop working. Instead of using createArrayBufferFromAddress, this PR changes to use type array's constructor to create a view of the underlying buffer. As a side effect, there should also no longer be a need to manually free the underlying buffer, v8 should automatically free it when there is no more references.

Tests are failing on node >= v14.9 due to https://github.com/nodejs/node/issues/35620. And is failing on < v14.9 possibly due to https://github.com/lovell/sharp/issues/2196. The fix for the former should be released in the next version so I will put this on hold until that happens.

TODO: Test for leaks with asan.

minggangw commented 3 years ago

So I reckon this PR could also work properly on node < 14 (before change of ArrayBuffer::New), right?

koonpeng commented 3 years ago

I don't see any reason for it to break on older versions, but I haven't tested it yet.

I'm looking at how I can make automated asan test easier, I'm getting lots of noise from other modules currently.

minggangw commented 3 years ago

Great! :+1:

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 91.747% when pulling c924a369dd4460f9050f75839b78d58210fac6a2 on fix/node-14 into 6f8a1a156780b131823e48650a35d9ff4760e462 on develop.

minggangw commented 3 years ago

Is it possible that the crash is caused by node-ffi-napi, please check out https://github.com/node-ffi-napi/ref-napi/issues/47

koonpeng commented 3 years ago

Is it possible that the crash is caused by node-ffi-napi, please check out node-ffi-napi/ref-napi#47

It does look like the same bug, hopefully it will work be fixed in the next release of ref-napi. When that happens I will test if the changes in this PR is needed, in the mean time I think I will continue leaving this as a draft.

minggangw commented 3 years ago

Hope so, thanks!

csmith-rtr commented 2 years ago

@minggangw @koonpeng are you planning to complete the transition to Node v14? Support for Node v12 is ending in April 2022. At this point, it might make more sense to go straight to v16.

wayneparrott commented 2 years ago

@csmith-rtr Work on this has been progressing. See the node-16 branch https://github.com/RobotWebTools/rclnodejs/tree/nodejs-16 that is under development. I've been running it for the past 2 weeks with node 12 & 16 on foxy and galactic with no obvious issues. Hopefully it will merge to head (develop branch) soon.

csmith-rtr commented 2 years ago

great! I hadn't seen that branch.