refactor: require native shared components implementation

[Vickysomtee]

The PR provides the guide to set up indy-vdr optional peer dependencies

Work funded by the Government of Ontario

[codecov-commenter]

Codecov Report

Merging #1349 (dad8884) into main (5559996) will increase coverage by 0.01%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##             main    #1349      +/-   ##
==========================================
+ Coverage   85.31%   85.32%   +0.01%     
==========================================
  Files         787      788       +1     
  Lines       19404    19405       +1     
  Branches     3150     3150              
==========================================
+ Hits        16555    16558       +3     
+ Misses       2842     2840       -2     
  Partials        7        7              

Impacted Files	Coverage Δ
packages/indy-vdr/src/IndyVdrModule.ts	92.30% <ø> (+14.52%)	⬆️
packages/indy-vdr/src/IndyVdrModuleConfig.ts	71.42% <0.00%> (-28.58%)	⬇️
packages/askar/src/AskarModuleConfig.ts	60.00% <60.00%> (ø)
packages/askar/src/AskarModule.ts	88.23% <100.00%> (+16.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[genaris]

Just a few typos in the IndyVdrModuleConfig comments.

[TimoGlastra]

Some final comments

[TimoGlastra]

Yeah it has to be something with this branch. All other PRs have no issues with CI

We switched indy-vdr from v9 to v10 in this branch. Let's see if it resolves the issue if we downgrade.

[genaris]

Yeah it has to be something with this branch. All other PRs have no issues with CI

We switched indy-vdr from v9 to v10 in this branch. Let's see if it resolves the issue if we downgrade.

D'oh haven't seen this and I upgraded again to v10 in #1373 😞. Anyway, there it worked fine at the first try.

[TimoGlastra]

Yeah seems that downgrading didn't do the trick. So it's something else, which is weird!?!?

Or maybe it's because we're now importing the node.js modules more often, which results in the memory issues?

[genaris]

These imports in tests' setup.ts are not needed anymore, are they?

[TimoGlastra]

They are! Some tests don't use the module (e.g. IndyVdrSovDidResolver.test.ts), but do use the binary. It will result in the binary not being registered on the shared package if you run the test (mainly in isolation).

I first started importing the nodejs bindings in these specific files, but I thought why make it more complex than needs to be and just imported it in the setup

[genaris]

Yeah seems that downgrading didn't do the trick. So it's something else, which is weird!?!?

Or maybe it's because we're now importing the node.js modules more often, which results in the memory issues?

This is absolutely possible, as in the wrappers we only provide a method to load libraries but not free resources used by them (which is certainly a bug, unless the runtime is freeing them automatically... which is not usually the case from our recent experience). ffi-napi provides ways to unload libraries, so I'll dig a bit more on it to see how it works and if we can solve it in a relatively easy way.

[TimoGlastra]

Yeah seems that downgrading didn't do the trick. So it's something else, which is weird!?!?
Or maybe it's because we're now importing the node.js modules more often, which results in the memory issues?

This is absolutely possible, as in the wrappers we only provide a method to load libraries but not free resources used by them (which is certainly a bug, unless the runtime is freeing them automatically... which is not usually the case from our recent experience). ffi-napi provides ways to unload libraries, so I'll dig a bit more on it to see how it works and if we can solve it in a relatively easy way.

We should maybe add a check and see if it's already loaded, and if so, don't load it again. I wouldn't expect importing a package multiple times has implications. If you're working in Node.JS you could only work with the Node.JS package, and never add / import the shared package...

[TimoGlastra]

100th time's the charm 🤞

[TimoGlastra]

Never thought this day would come

[genaris]

Never thought this day would come

The only thing we had to do was to rename it! Let's keep this in mind for the next cursed PR.