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

✨feat(llm/lld): avoid watchloop when ff is off #7601

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

LucasWerey
Copy link
Contributor

@LucasWerey LucasWerey commented Aug 14, 2024

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • LS
    • Jest config
    • Test utils

📝 Description

We don't want to run the LS watchloop when the ff is disabled on LLM and LLD

Note

  • On LLD
    Jest config has been updated also to be able to run pnpm desktop test:jest _fileName_ and to exclude all shared files from test on LLD as it's done on LLM
    Add a custom resolver for the firebase feature flag also for Jest so we can access them during tests
    Add fireBase provider to renderHook of test utils
    Move helpers from testHelper to shared
    RecipientField tests have been updated with a mocked FirebaseFeatureFlagsProvider so tests don't failed
  • ON LLM
    Add renderHook to test-renderer to be able to test hooks
    Mock LS for tests

Warning

WIP

  • A test on LLD and on LLM is skipped because it loops infinitely so it needs to be fixed. It needs some investigation on the WATCHLOOP to know why it's looping infinitly caused by setOnUserRefresh(() => onUserRefreshIntent);
    I suppose that it's caused by the fact we use some timeout or by the fact we called a set inside the useEffect
    A possibility would be to use useRef instead of useState for setOnUserRefresh this way the test doesn't loop. I've pushed this solution under this commit. It looks like the behaviour remains the same.

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@LucasWerey LucasWerey requested a review from a team as a code owner August 14, 2024 09:27
Copy link

vercel bot commented Aug 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

5 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-docs ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 3:16pm
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 3:16pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 3:16pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 3:16pm
web-tools ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 3:16pm

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM labels Aug 14, 2024
thesan
thesan previously approved these changes Aug 14, 2024
Copy link
Contributor

@thesan thesan left a comment

Choose a reason for hiding this comment

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

LGTM!

@LucasWerey LucasWerey added the WIP Work in progress label Aug 14, 2024
@LucasWerey LucasWerey force-pushed the feat/LIVE-13546-c branch 2 times, most recently from 67c6213 to 105774c Compare August 16, 2024 11:55
@LucasWerey LucasWerey removed the WIP Work in progress label Aug 16, 2024
@cgrellard-ledger cgrellard-ledger merged commit d75616a into develop Aug 20, 2024
47 of 50 checks passed
@cgrellard-ledger cgrellard-ledger deleted the feat/LIVE-13546-c branch August 20, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Has changes in LLD mobile Has changes in LLM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants