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

fix: JS wrapper updates #231

Merged
merged 5 commits into from
Aug 17, 2023
Merged

Conversation

genaris
Copy link
Contributor

@genaris genaris commented Aug 12, 2023

Some updates to JavaScript wrappers to match main branch status:

I went a bit more further and updated minimum node version to 18, which is in fact the minimum usable version due to the known performance issue of ref-napi (which is patched). node 16 will reach its EOL next month anyway.

@@ -55,7 +55,10 @@
"tsconfig-paths": "4.1.0",
"typescript": "4.5.5"
},
"resolutions": {
"ref-napi": "npm:@2060.io/ref-napi"
Copy link
Member

Choose a reason for hiding this comment

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

Can't we add this as a direct dependency for all the wrappers and not require the resolution anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if that would work because it is also an indirect dependency: ffi-napi requires it. In such case maybe we can also patch it to depend on the patched ref-napi as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimoGlastra what about these latest changes? I needed some tweaks for types, but at the end seems to work fine. I think it's worth to put some effort on this considering the several problems users had in setting up these components for NodeJS.

Copy link
Member

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

This is great! Would definitely help in making it easier to use these libraries (we should probably do it for all libraries).

Maybe we can also look at not hosting it under 2060 npm scope, but that's something we can look at later on (not sure what would be a good home)

@@ -297,7 +297,6 @@ describe('bindings', () => {
revocationRegistryDefinition,
revocationRegistryDefinitionPrivate,
registryIndex: 9,
tailsPath,
Copy link
Member

Choose a reason for hiding this comment

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

tailsPath is not used anymore (eslint warning)

@TimoGlastra TimoGlastra merged commit 74aa78a into hyperledger:main Aug 17, 2023
25 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.

2 participants