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

build(deps): replace ipfs-http-client with kubo-rpc-client #2829

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Oct 24, 2023

  • Replace deprecated ipfs-http-client with kubo-rpc-client.
  • kubo-rpc-client must be imported dynamically since it's ESM-only and we
    still use CJS.

Peter's additional changes:

build(typescript): project-wide fixes to allow us to use ESM-only deps

Apologies for the huge diff, this can't be broken down to smaller changes
that would still compile because of cross-package dependencies.

I realize that this change is not exactly the optimal solution, but
it is probably a step in the right direction.
If I somehow found the time to submit pull requests to the libraries
that I needed to fork and re-publish (see details below) and then get
the changes onto the upstream and get them released as the official
packages, then we could (in theory) arrive at a solution that is the
recommended way of fixing these problems (apart from going full ESM-only)

This work stands on the shoulders of the previous commits from @outSH
and takes a slightly different direction compared to what we've been
talking about earlier on account of the problem that the eval-import
workaround causes crashes in Jest.

Based on the above I went through the following adventures:

  1. I migrated the build system of kubo-rpc-client myself so that it
    correctly exports CJS and ESM and typings for both of those as well,
    I put that code on my fork [1] and then published it onto npm as well [2]
    After this, I was hoping that now we could just import the package in
    our CJS code without issues, but what really happened is that instead of
    crashing at the require call that pull in kubo itself, it started crashing
    deeper in the require stack where kubo itself was requiring it's own
    ESM only dependencies (of which there seem to be at least 10 or 15).
    At this point I realized that me migrating and self-publishing all of
    these additional packages might not be worth the effort and started looking
    for something easier.
  2. I gave dynamic imports + moduleResultion=Node16 as my next attempt to
    get our build back to working order. With this, the kubo-rpc-client
    can now be imported dynamically without issues in packages that declare
    themselves as resolving modules as "Node16" in their tsconfig.json
    Other issues here were encountered because not all of our ESM only packages
    are used in a way that they can be imported dynamically (for example
    if their types are part of our own types or are being re-exported).
    The two libraries with this problem were run-time-error and
    socket.io-client for both of which I ended up going through the same
    treatment as for kubo-rpc-client above (but this time my effort wasn't)
    in vain. They work and so I did some search and replace in the entire
    codebase to use these re-published packages with the correct types:
    [3] [4] [5] [6]
  3. After this the project build was working, but Jest was still failing with
    compiler errors which I determined to happen because it uses the
    root tsconfig.json file for it's internal TS compilation and that root
    tsconfig.json file was not setting module resolution to Node16.
  4. After fixing that the final hurdle (hopefully) was to ensure that jest gets
    execued with the custom node option as below:
    NODE_OPTIONS=--experimental-vm-modules yarn jest

[1] https://github.com/petermetz/js-kubo-rpc-client-esm-cjs
[2] https://www.npmjs.com/package/kubo-rpc-client-esm-cjs

[3] https://github.com/petermetz/socket.io-client
[4] https://www.npmjs.com/package/socket.io-client-fixed-types
[5] https://github.com/petermetz/RuntimeError
[6] https://www.npmjs.com/package/run-time-error-cjs

Huge thanks for https://arethetypeswrong.github.io/ a tool I used
extensively to create the fixes for the libraries above.

One more thing that I tried just to collect more data points was to
set the moduleResultion project-wide to Node16 via setting it
in the root tsconfig.base.json but this broke the compiler itself,
as in, there is a bug in the Typescript compiler in v4.x as seen here:
microsoft/TypeScript#51221
So this is one more reason for us to upgrade to 5.x as soon as possible.

I also needed to add "run-time-error" to the root package.json as a
dependency because it was accidentally providing that to some sub-packages
and when we moved to "run-time-error-cjs" the tests that install
plugins from npm started failing (because those releases are still
using "run-time-error" and not "run-time-error-cjs")


Fixes #2807
Fixes #2852

Depends on: #2821

Co-authored-by: Peter Somogyvari [email protected]

Signed-off-by: Peter Somogyvari [email protected]
Signed-off-by: Michal Bajer [email protected]

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@outSH
Copy link
Contributor Author

outSH commented Oct 24, 2023

@petermetz

Unfortunatelly, in our case kubo-rpc-client is not a drop-in replacement because this new package supports ESM modules only. I couldn't find any workaround for this, other than importing this package dynamically. This didn't work too well either, because:

  1. A bug in Typescript that transpiles dynamic import into... dynamic require, that brings us back to the problem (see Add flag to not transpile dynamic import() when module is CommonJS microsoft/TypeScript#43329)
  2. We can't import types from dynamic / ESM import.

Workarounds:
ad1 - I've used a workaround from linked issue where we evaluate the import statement, that way it bypass transpilation and work just fine, but it looks fishy.
ad2 - I've copied some types from kubo-rpc-client, and added LikeIpfsHttpClient interface that checks only methods we care about in this connector.

Let me know if that looks OK for you and will replace ipfs-http-client with kubo-rpc-client in other modules as well (it's used mostly in test, so there will be issue 1 only)

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH Agreed, we don't have a choice but to eval-import. :-(

The usage of eval will probably set off a thousand alarms on all the security tools that we have in place, so what I would recommend on the implementation details of how we do the eval is this list:

  1. Centralize it: have a dedicated function doing the eval import in maybe cactus-common or cactus-core or even a new cacti-esm-compat-hacks package (the separate package has a lot of overhead but it is nice that in the future if we want to get rid of the eval we don't have to do a breaking API change on cactus-common/cactus-core)
  2. Keep it hardcoded despite it being a dedicated function: As-in, resist the urge to make it a generic eval import function that can eval-import any provided package through a string parameter.
  3. Name the function something that communicates that it shouldn't be used like evalImportKuboRpcClientDeprecated() or something similar (deprecated being a hint that it should only be used when there is no other choice)
  4. When the function is called, it should print on console.warn and do a bunch of self-shaming about what it's doing and that this should be fixed and maybe even a link to this PR/related discussions in the issue(s) we've had about it. My intention here being that if someone brand new comes in to the project they should be able to see immediately why we were forced to do what we did and not waste time on going down the same rabbit-hole
  5. The logging above will also make sure that we do not forget to take care of this somehow (the only way forward seems like migrating to ESM - which is something we should consider among all the maintainers)

Finally: the rest of the occurrences would also need to be migrated (let me know if you want help with this mundane chore, I'm happy to collaborate on the PR)

image

One last thing: Some of the dependencies that you've added/updated in extensions/cactus-plugin-object-store-ipfs/package.json file are not the latest ones. If there is no specific reason to not use the absolute latest ones then I'd suggest using the latest ones just to (hopefully) increase the time between now and when we have to touch these again due to vulnerabilities/deprecation/etc.

image


I'm happy to help with all of these too, just let me know if you'd rather share the load!

@outSH outSH force-pushed the ipfs_replacement_wip branch from 3bd88c0 to bcad3be Compare October 25, 2023 07:24
@outSH
Copy link
Contributor Author

outSH commented Oct 25, 2023

@petermetz

Centralize it: have a dedicated function doing the eval import in maybe cactus-common or cactus-core or even a new cacti-esm-compat-hacks package

I went with a separate package because I didn't want to introduce dependency to kubo rpc in any of the common packages. I've tried to follow your suggestions as well.

But, there's another bump on the road - dynamic import of ESM module segfaults when performed inside of Jest test (seem to work with tape) 😫
It may be caused by a bug in nodejs itself (nodejs/node#35889?). After upgrade to node V20 segfaults disappears but there are some warnings about ESM being experimental in jest (https://jestjs.io/docs/ecmascript-modules) and most tests fail (see commit 2 for reproduction)

I will also add additional warning message to dynamicImportKuboRpcClientESMWorkaround function informing that tests may crash when this is used, and will try to remove ipfs from these tests (if possible)

Finally: the rest of the occurrences would also need to be migrated (let me know if you want help with this mundane chore, I'm happy to collaborate on the PR)

No worries, I'll let you know if I need help on this :)

One last thing: Some of the dependencies that you've added/updated in extensions/cactus-plugin-object-store-ipfs/package.json file are not the latest ones

Yes, that is intentionally. These packages are the same as in kubo-rpc-client to match their interfaces. They are defined as dev dependencies and used only to fetch type definitions (i.e. import type) so even if they are outdated they are not a real security issue. Alternatively I can replace problematic fields with unknown or any, but we lose some type safety this way so I'm not sure if that's better.

@outSH
Copy link
Contributor Author

outSH commented Oct 26, 2023

@petermetz I've tried alternative ways to do this dynamic import but it ends up with jest segfaulting all the time :/ Can you give it a shot?

For now I see the following options:

  • Crate issue for kubo-rpc to build both ESM and CJS (like most projects). This will probably take time to discuss, confirm and we may end up preparing the PR ourselves.
  • Allow ipfs-http-client only as devDependency.
  • Move on with migration to ESM in cacti, this may take lots of time + openapi gen tools doesn't support ESM imports as well, so we'd have to figure that out (ESM imports => relative paths must contain extension, like ../../api.js instead of ../../api).

@outSH outSH force-pushed the ipfs_replacement_wip branch from 70cd17a to bcad3be Compare October 26, 2023 13:45
@petermetz
Copy link
Contributor

petermetz commented Nov 1, 2023

@petermetz I've tried alternative ways to do this dynamic import but it ends up with jest segfaulting all the time :/ Can you give it a shot?

For now I see the following options:

* Crate issue for kubo-rpc to build both ESM and CJS (like most projects). This will probably take time to discuss, confirm and we may end up preparing the PR ourselves.

* Allow ipfs-http-client only as devDependency.

* Move on with migration to ESM in cacti, this may take lots of time + openapi gen tools doesn't support ESM imports as well, so we'd have to figure that out (ESM imports => relative paths must contain extension, like ../../api.js instead of ../../api).

@outSH I did, massive rabbit hole! I found a workaround that might be okay that involves dynamic imports, module resolution set to Node16 and me manually fixing the builds/typings of a few libraries and republishing them onto npm. Much more details about all this in the commit message that I just pushed. I'm going to bed now but hopefully I wake up to the tests working (or even at least mostly working would be great at this point...)

Pasting the commit message here for better visibility:


build(typescript): project-wide fixes to allow us to use ESM-only deps

Apologies for the huge diff, this can't be broken down to smaller changes
that would still compile because of cross-package dependencies.

I realize that this change is not exactly the optimal solution, but
it is probably a step in the right direction.
If I somehow found the time to submit pull requests to the libraries
that I needed to fork and re-publish (see details below) and then get
the changes onto the upstream and get them released as the official
packages, then we could (in theory) arrive at a solution that is the
recommended way of fixing these problems (apart from going full ESM-only)

This work stands on the shoulders of the previous commits from @outSH
and takes a slightly different direction compared to what we've been
talking about earlier on account of the problem that the eval-import
workaround causes crashes in Jest.

Based on the above I went through the following adventures:

  1. I migrated the build system of kubo-rpc-client myself so that it
    correctly exports CJS and ESM and typings for both of those as well,
    I put that code on my fork [1] and then published it onto npm as well [2]
    After this, I was hoping that now we could just import the package in
    our CJS code without issues, but what really happened is that instead of
    crashing at the require call that pull in kubo itself, it started crashing
    deeper in the require stack where kubo itself was requiring it's own
    ESM only dependencies (of which there seem to be at least 10 or 15).
    At this point I realized that me migrating and self-publishing all of
    these additional packages might not be worth the effort and started looking
    for something easier.
  2. I gave dynamic imports + moduleResultion=Node16 as my next attempt to
    get our build back to working order. With this, the kubo-rpc-client
    can now be imported dynamically without issues in packages that declare
    themselves as resolving modules as "Node16" in their tsconfig.json
    Other issues here were encountered because not all of our ESM only packages
    are used in a way that they can be imported dynamically (for example
    if their types are part of our own types or are being re-exported).
    The two libraries with this problem were run-time-error and
    socket.io-client for both of which I ended up going through the same
    treatment as for kubo-rpc-client above (but this time my effort wasn't)
    in vain. They work and so I did some search and replace in the entire
    codebase to use these re-published packages with the correct types:
    [3] [4] [5] [6]
  3. After this the project build was working, but Jest was still failing with
    compiler errors which I determined to happen because it uses the
    root tsconfig.json file for it's internal TS compilation and that root
    tsconfig.json file was not setting module resolution to Node16.
  4. After fixing that the final hurdle (hopefully) was to ensure that jest gets
    execued with the custom node option as below:
    NODE_OPTIONS=--experimental-vm-modules yarn jest

[1] https://github.com/petermetz/js-kubo-rpc-client-esm-cjs
[2] https://www.npmjs.com/package/kubo-rpc-client-esm-cjs

[3] https://github.com/petermetz/socket.io-client
[4] https://www.npmjs.com/package/socket.io-client-fixed-types
[5] https://github.com/petermetz/RuntimeError
[6] https://www.npmjs.com/package/run-time-error-cjs

Huge thanks for https://arethetypeswrong.github.io/ a tool I used
extensively to create the fixes for the libraries above.

One more thing that I tried just to collect more data points was to
set the moduleResultion project-wide to Node16 via setting it
in the root tsconfig.base.json but this broke the compiler itself,
as in, there is a bug in the Typescript compiler in v4.x as seen here:
microsoft/TypeScript#51221
So this is one more reason for us to upgrade to 5.x as soon as possible.

@petermetz petermetz force-pushed the ipfs_replacement_wip branch 2 times, most recently from 7e80682 to ab2c112 Compare November 1, 2023 18:28
@outSH
Copy link
Contributor Author

outSH commented Nov 3, 2023

@petermetz Thanks, this looks great :) I'm somewhat afraid of using own releases of 3'rd party packages, but if you're willing to keep them updated with upstream releases then this may work for now. I'd still consider moving to ESM soon, given that more and more packages are dropping CJS completely.

@petermetz
Copy link
Contributor

I'm somewhat afraid of using own releases of 3'rd party packages, but if you're willing to keep them updated with upstream releases then this may work for now.

@outSH Agreed, 100%. I'm not planning on maintaining these forever (not even long term). What I'm hoping is that it's just the duct-tape that will hold things over until these build changes in these projects can be upstream'd to the official repos and then we can delete & unpublish my forks entirely.

I'd still consider moving to ESM soon, given that more and more packages are dropping CJS completely.

Similar thoughts here. If @VRamakrishna and @sandeepnRES also agree, then I see no reason why not do it as soon as possible (I definitely won't have time for the next 4 or so weeks but happy to give it a shot later)

@petermetz petermetz self-requested a review November 4, 2023 03:13
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH It looks like this is ready for prime-time now, so I'm dropping an approval, feel free to dissolve my commit into your original one; I'll just recommend that we keep the description in the commit message as a specific paragraph of your commit message at least so that maybe others reading in the future will learn from our adventures.

@petermetz petermetz marked this pull request as ready for review November 6, 2023 20:44
@petermetz petermetz force-pushed the ipfs_replacement_wip branch from ab2c112 to 59d486c Compare November 10, 2023 17:01
@petermetz
Copy link
Contributor

@outSH I did a squash so that my changes are part of your commit and also disappeared the temporary commit in-between the two, so now it's ready for merge as soon as we have enough reviews!

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

- Replace deprecated ipfs-http-client with kubo-rpc-client.
- kubo-rpc-client must be imported dynamically since it's ESM-only and we
    still use CJS.

Peter's additional changes:
---------------------------

build(typescript): project-wide fixes to allow us to use ESM-only deps

Apologies for the huge diff, this can't be broken down to smaller changes
that would still compile because of cross-package dependencies.

I realize that this change is not exactly the optimal solution, but
it is probably a step in the right direction.
If I somehow found the time to submit pull requests to the libraries
that I needed to fork and re-publish (see details below) and then get
the changes onto the upstream and get them released as the official
packages, then we could (in theory) arrive at a solution that is the
recommended way of fixing these problems (apart from going full ESM-only)

This work stands on the shoulders of the previous commits from @outSH
and takes a slightly different direction compared to what we've been
talking about earlier on account of the problem that the eval-import
workaround causes crashes in Jest.

Based on the above I went through the following adventures:
1. I migrated the build system of kubo-rpc-client myself so that it
correctly exports CJS and ESM and typings for both of those as well,
I put that code on my fork [1] and then published it onto npm as well [2]
After this, I was hoping that now we could just import the package in
our CJS code without issues, but what really happened is that instead of
crashing at the require call that pull in kubo itself, it started crashing
deeper in the require stack where kubo itself was requiring it's own
ESM only dependencies (of which there seem to be at least 10 or 15).
At this point I realized that me migrating and self-publishing all of
these additional packages might not be worth the effort and started looking
for something easier.
2. I gave dynamic imports + moduleResultion=Node16 as my next attempt to
get our build back to working order. With this, the kubo-rpc-client
can now be imported dynamically without issues in packages that declare
themselves as resolving modules as "Node16" in their tsconfig.json
Other issues here were encountered because not all of our ESM only packages
are used in a way that they can be imported dynamically (for example
if their types are part of our own types or are being re-exported).
The two libraries with this problem were `run-time-error` and
`socket.io-client` for both of which I ended up going through the same
treatment as for kubo-rpc-client above (but this time my effort wasn't)
in vain. They work and so I did some search and replace in the entire
codebase to use these re-published packages with the correct types:
[3] [4] [5] [6]
3. After this the project build was working, but Jest was still failing with
compiler errors which I determined to happen because it uses the
root tsconfig.json file for it's internal TS compilation and that root
tsconfig.json file was not setting module resolution to Node16.
4. After fixing that the final hurdle (hopefully) was to ensure that jest gets
execued with the custom node option as below:
NODE_OPTIONS=--experimental-vm-modules yarn jest

[1] https://github.com/petermetz/js-kubo-rpc-client-esm-cjs
[2] https://www.npmjs.com/package/kubo-rpc-client-esm-cjs

[3] https://github.com/petermetz/socket.io-client
[4] https://www.npmjs.com/package/socket.io-client-fixed-types
[5] https://github.com/petermetz/RuntimeError
[6] https://www.npmjs.com/package/run-time-error-cjs

Huge thanks for https://arethetypeswrong.github.io/ a tool I used
extensively to create the fixes for the libraries above.

One more thing that I tried just to collect more data points was to
set the moduleResultion project-wide to Node16 via setting it
in the root tsconfig.base.json but this broke the compiler itself,
as in, there is a bug in the Typescript compiler in v4.x as seen here:
microsoft/TypeScript#51221
So this is one more reason for us to upgrade to 5.x as soon as possible.

I also needed to add "run-time-error" to the root package.json as a
dependency because it was accidentally providing that to some sub-packages
and when we moved to "run-time-error-cjs" the tests that install
plugins from npm started failing (because those releases are still
using "run-time-error" and not "run-time-error-cjs")
------------------------------------

Fixes hyperledger-cacti#2807
Fixes hyperledger-cacti#2852

Depends on: hyperledger-cacti#2821

Co-authored-by: Peter Somogyvari <[email protected]>

Signed-off-by: Peter Somogyvari <[email protected]>
Signed-off-by: Michal Bajer <[email protected]>
@petermetz petermetz force-pushed the ipfs_replacement_wip branch from 59d486c to 8800d7d Compare November 15, 2023 22:30
@petermetz petermetz merged commit eac2201 into hyperledger-cacti:main Nov 15, 2023
17 of 119 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
3 participants