Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: migrate from ffi-napi to koffi for foreign function calls (CON-304) #203

Merged

Conversation

RobbertProost
Copy link
Contributor

As described in issue #198, the javascript connector is no longer compatible with newer versions of Node due to the ffi-napi package not being compatible.

ffi-nap seems no longer being maintained given that the last contribution is from more than two years ago.

An alternative package that provides similar functionallity is koffi. The syntax is very similar to ffi-napi and is currently actively being maintained.

This MR migrates from ffi-napi to koffi library, removing the need for the incompatible ffi-napi package.

I have tested these changes with v1.3.0 and v1.2.2 of the RTIConnextDDS-Connector i.c.w. v20.13.1 of Node on Linux. The examples work as expected and all tests in the test suite pass.

I don't have access to your CI pipelines, so would be great if you could run them on other Node versions and other platforms as well.

In terms of API there are two changes:

  • The exposed RTI functions or no longer part of the api object inside the _ConnectorBinding, but are direct methods inside the _ConnectorBinding class
  • Functions returning a boolean actually return a boolean now, instead of a 1 or 0

Please let me know if you are willing to accept this change or what would need to change to get this in.

@alexcamposruiz
Copy link
Collaborator

Thanks for the pull request! We will review it and test it.

@alexcamposruiz alexcamposruiz self-requested a review May 21, 2024 18:21
@samuelraeburn samuelraeburn self-requested a review May 23, 2024 14:26
Copy link
Collaborator

@alexcamposruiz alexcamposruiz left a comment

Choose a reason for hiding this comment

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

We've reviewed the changes and they look good. All our tests are passing. I'm going to merge the PR. Thanks again.

@alexcamposruiz alexcamposruiz merged commit c6a83c6 into rticommunity:develop Jun 5, 2024
6 checks passed
alexcamposruiz pushed a commit that referenced this pull request Jun 5, 2024
alexcamposruiz added a commit that referenced this pull request Jun 5, 2024
@RobbertProost RobbertProost deleted the migrate-to-koffi branch June 8, 2024 06:15
@Alxe
Copy link
Member

Alxe commented Jun 14, 2024

@RobbertProost Thank you for your contribution. :)

@Alxe Alxe changed the title refactor: migrate from ffi-napi to koffi for foreign function calls refactor: migrate from ffi-napi to koffi for foreign function calls (CON-304) Jul 31, 2024
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.

3 participants