fix: abstract out circular dependencies between engine and networks util

tommasini commented 4 days ago

Description

Added dpdm to measure circular dependencies: https://github.com/acrazing/dpdm
Fixed circular dependencies between engine -> network/utils
Add script using dpdm to package json circular:deps

This still have circular deps between the new engineNetworkUtils and Engine, they will be a follow up discussion, since the code in Engine might not be needed anymore

PR - https://www.notion.so/metamask-consensys/Remove-all-circular-dependencies-in-the-codebase-30-in-total-to-enable-HMR-and-React-Refresh-Mo-02737ed3b49c4702b8cb55fdb67020e8

Related issues

Fixes:

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

[ ] I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
[ ] I've completed the PR template to the best of my ability
[ ] I’ve included tests if applicable
[ ] I’ve documented my code using JSDoc format if applicable
[ ] I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

[ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
[ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions[bot] commented 4 days ago

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

github-actions[bot] commented 4 days ago

Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: f2035297d8f6e25549aacdbc7a1fc490a4ad43be Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/fd2e94b1-6492-4c33-8c4d-b8e6d1648db9

[!NOTE]

This comment will auto-update when build completes

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

socket-security[bot] commented 4 days ago

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/dpdm@3.14.0	filesystem, unsafe	`0`	265 kB	acrazing
npm/package-json-from-dist@1.0.1	None	`0`	36.5 kB	isaacs
npm/tslib@2.8.1	None	`0`	90.4 kB	typescript-bot
npm/typescript@5.6.3	None	`0`	22.4 MB	typescript-bot

🚮 Removed packages: npm/minipass@7.1.1, npm/tslib@2.6.3

View full report↗︎

github-actions[bot] commented 4 days ago

Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 55207884da85c0e752e14dd12411bc1e5297583e Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c80fbb88-d61f-4396-970a-bd217d8a3e9c

[!NOTE]

This comment will auto-update when build completes

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

wachunei commented 4 days ago

awesome!

I suggest adding this to the description https://www.notion.so/metamask-consensys/Remove-all-circular-dependencies-in-the-codebase-30-in-total-to-enable-HMR-and-React-Refresh-Mo-02737ed3b49c4702b8cb55fdb67020e8.

hey @gauthierpetetin, do you know there's a mobile specific ticket for this?

github-actions[bot] commented 4 days ago

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 616629bde37b77d6c384e5df8cdca22b7d0f8e29 Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/4212ecfa-d721-444b-9fee-82604002ff4c

[!NOTE]

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

tommasini commented 4 days ago

@wachunei there is not yet! but there will be a ticket for this soon! I will send to you, when I create, just trying to understand the best strategy to address those and getting my hands on it on the way :p