Countless dependency cycles in core package

[PeterBenc]

Summary

I am trying to use cardano-sdk in SES environment. I am unable to do so getting

Class extends value undefined is not a constructor or null

Most likely, this is because of dependency cycles. I tried using madge which found ~165 dependency cycles in the core package. Most of them I suspect, can be fixed with import type.

Steps to reproduce the bug

Run
yarn madge --circular src/index.ts --debug --extensions ts in packages/core

Actual Result

`✖ Found 165 circular dependencies!

Expected Result

Found 0 circular dependencies!

SDK version

0.28.4

Environment type

• Node.js
• Browser
• Browser Extension
• Other

Environment details

No response

[iccicci]

We actaully have a circular dependency check... it seems the one you run is stronger than the one currently implemented; we'll take a look.

Once said that, it seems you called "steps to reproduce the bug" the check you performed to discover circular dependencies: could you please tell which is the command which gives you the first error reported?

Thank you

[PeterBenc]

Thanks for the response, we are building a Metamask snap for Cardano, and I get the error when building the snap, just running yarn build. I don't see much in the insides of the building or the SES environment. It would IMO make sense to look deeper into it if fixing the circ. dependencies did not fix the issue.

Btw some of the circ. dependencies are quite direct

1. Cardano/Address/Address.ts > Cardano/Address/BaseAddress.ts
2. Cardano/Address/Address.ts > Cardano/Address/ByronAddress.ts

[mkazlauskas]

Thank you for a report! It seems like core package.json is missing a circular-deps:check script that is present in other packages, so we didn't catch those issues 🤦

We will fix it, but can't make any promises on the timeline. In case you need this resolved asap, code contributions are welcome

[PeterBenc]

Thanks, I will consider fixing it myself 👍 Since it should be "just moving stuff around", would you be up for making a quick npm release for that or is there some time-consuming process for making releases?

[mkazlauskas]

Publishing packages is one-click, no problem :) If possible, best to not make any breaking changes when moving stuff around

[PeterBenc]

Hmm, I don't see it working without breaking changes, you can create Address in BaseAddress and BaseAddress in Address. Would you open to release also with breaking changes? I am working on it, but I can already see that at least this needs to be changed.

[mkazlauskas]

Perhaps you could create a list of remaining issues that you couldn't solve without breaking changes, I'll take care of those

[PeterBenc]

That would be great, thanks. The thing is tho, I need to have it, just to try the package and move forward with my other work. If its ok, I ll try fixing everything, also with breaking changes, possibly open PR. You can then decide to reuse what makes sense to you and change what does not. This way I can move forward and you can have control of how the breaking changes look, without me pinging you every day waiting for it to be ready 😄

[rhyslbw]

Thanks @PeterBenc, yes this is a good approach. We're a little behind on contributor documentation but please be aware we're implementing the Conventional Commits standard with atomic commits.

It's great to see you considering this SDK for your project. Please feel free to open any discussions during your evaluation

[PeterBenc]

Update
It turned out that the dependency cycles were not the issue, SES env I am working with does not include built-in websocket which caused the error, isomoric-ws extends the websocket, thus the Class extends value undefined is not a constructor or null.

I fixed the issue by mocking the isomorfic-ws package in webpack.

Regarding the dependency cycles. I fixed them all. But honestly, nothing that I would be proud to share. More time and changes are needed for the proper solution.

[mkazlauskas]

Thank you for sharing the solution