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

[EPIC]: numerous "modules cycles", AKA circular dependencies #22450

Open
davidmurdoch opened this issue Jan 8, 2024 · 3 comments
Open

[EPIC]: numerous "modules cycles", AKA circular dependencies #22450

davidmurdoch opened this issue Jan 8, 2024 · 3 comments
Assignees
Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. ready-for-dev regression-prod-* Regression bug that was found in production in release * Sev2-normal Normal severity; minor loss of service or inconvenience. team-tiger Tiger team (for tech debt reduction + performance improvements) type-bug type-tech-debt Technical debt

Comments

@davidmurdoch
Copy link
Contributor

Describe the bug

Probably more a of a bug lurking, since circular dependencies make code fragile.

Node.js doc state:

Careful planning is required to allow cyclic module dependencies to work correctly within an application.

https://nodejs.org/api/modules.html#modules_cycles

In the branch I'm testing, there are at least 30 module cycles in the extension codebase.

To get an initial list run npx madge --circular --extensions ts,js ./ at the root of the repo.

Beyond the dangers of hidden/waiting bugs, the cycles prevent HMR (hot module reloading) and React Refresh from working. This issue is likely contributing to longer load times for users and longer extension build times for devs.

@brad-decker suggests, and I agree, that the cycles may also prevent tree-shaking and dead code removal from working fully.

Expected behavior

No response

Screenshots/Recordings

No response

Steps to reproduce

Run npx madge --circular --extensions ts,js ./ at the root of the repo.

Error messages or log output

No response

Version

Build type

Other (please specify exactly where you obtained this build in "Additional Context" section)

Browser

Other (please elaborate in the "Additional Context" section)

Operating system

Other (please elaborate in the "Additional Context" section)

Hardware wallet

No response

Additional context

No response

Severity

This affects everyone, including users (from the extension's size to load time to unknown/lurking bugs) and devs building MetaMask (as they can't use HMR).

It is tech debt and the bill is overdue.

@metamaskbot metamaskbot added the regression-prod-* Regression bug that was found in production in release * label Jan 8, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by severity Feb 19, 2024
@gauthierpetetin gauthierpetetin added the Sev2-normal Normal severity; minor loss of service or inconvenience. label Mar 25, 2024
@github-project-automation github-project-automation bot moved this to To be fixed in Bugs by team Apr 9, 2024
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Jun 23, 2024
@gauthierpetetin gauthierpetetin added type-tech-debt Technical debt team-tiger Tiger team (for tech debt reduction + performance improvements) and removed stale issues and PRs marked as stale labels Jul 29, 2024
@danjm danjm self-assigned this Jul 29, 2024
@danjm
Copy link
Contributor

danjm commented Jul 29, 2024

@brad-decker

Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Nov 28, 2024
@hjetpoluru hjetpoluru removed the stale issues and PRs marked as stale label Dec 17, 2024
@gauthierpetetin gauthierpetetin changed the title [Bug]: numerous "modules cycles", AKA circular dependencies [EPIC]: numerous "modules cycles", AKA circular dependencies Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-performance Issues relating to slowness of app, cpu usage, and/or blank screens. ready-for-dev regression-prod-* Regression bug that was found in production in release * Sev2-normal Normal severity; minor loss of service or inconvenience. team-tiger Tiger team (for tech debt reduction + performance improvements) type-bug type-tech-debt Technical debt
Projects
Status: To be fixed
Status: To be fixed
Status: To be fixed
Development

No branches or pull requests

5 participants