Fix store test circular dependency - Githubissues

MetaMask / metamask-mobile

Mobile web browser providing access to websites that use the Ethereum blockchain

https://metamask.io

Other

2.18k stars 1.12k forks source link

Fix store test circular dependency #8756

Open NicolasMassart opened 9 months ago

NicolasMassart commented 9 months ago

What is this about?

https://github.com/MetaMask/metamask-mobile/pull/8748/files#diff-60a04de3137b833473fa9964bfd0173a9607fc03abcf2b673b31d047ee305b27R23-R39

Scenario

No response

Design

No response

Technical Details

checkout main
yarn setup
jest app/components/UI/Ramp/Views/OrdersList/OrdersList.test.tsx
Test should pass
now just comment lines 25 on app/core/Analytics/index.ts
// export { store };
run jest app/components/UI/Ramp/Views/OrdersList/OrdersList.test.tsx again
Test should fail with No reducer provided for key "fiatOrders"

Add lint rule for catching cir deps - https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-cycle.md

Threat Modeling Framework

No response

Acceptance Criteria

store import and export removed from app/core/Analytics/index.ts
all unit test pass

Stakeholder review needed before the work gets merged

[X] Engineering (needed in most cases)
[ ] Design
[ ] Product
[X] QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
[ ] Security
[ ] Legal
[ ] Marketing
[ ] Management (please specify)
[ ] Other (please specify)

References

https://github.com/MetaMask/metamask-mobile/pull/8748/files#diff-60a04de3137b833473fa9964bfd0173a9607fc03abcf2b673b31d047ee305b27R23-R39

wachunei commented 8 months ago

Can we add https://github.com/MetaMask/metamask-mobile/pull/8138 as part of the issue and acceptance criteria?

wachunei commented 6 months ago

I'll leave two examples here, first:

Engine.ts imports util/networks https://github.com/MetaMask/metamask-mobile/blob/1232903b3dc98f428616c640810579020b40371a/app/core/Engine.ts#L127-L132
util/networks imports Engine.ts https://github.com/MetaMask/metamask-mobile/blob/1232903b3dc98f428616c640810579020b40371a/app/util/networks/index.js#L24

So here we see some utils are not pure and can be extracted as "Engine utils".

Second:

app/store/index.ts imports app/reducers/index.ts https://github.com/MetaMask/metamask-mobile/blob/1232903b3dc98f428616c640810579020b40371a/app/store/index.ts#L6
app/reducers/index.ts imports app/core/Engine.ts https://github.com/MetaMask/metamask-mobile/blob/1232903b3dc98f428616c640810579020b40371a/app/reducers/index.ts#L23
app/core/Engine.ts imports app/store/index.ts https://github.com/MetaMask/metamask-mobile/blob/1232903b3dc98f428616c640810579020b40371a/app/core/Engine.ts#L134