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

chore/nowarnings Solve some of the warnings we have on LLM #2732

Closed
wants to merge 8 commits into from

Conversation

juan-cortes
Copy link
Contributor

πŸ“ Description

Today I sat down and actually read the warnings that get thrown when we launch Ledger Live Mobile instead of dismissing them. How hard can it be to have an app without warnings or at least without errors? Let's go deeper.

  • 'deprecated-react-native-prop-types' warning, it doesn't seem to be coming from us and according to some guy on github it's safe to silence them.
  • Memory leak on remoteliveappprovider because we launch some async task and the component gets unmounted before it completes yet we try to update the state afterwards. Simply using the useIsMounted hook that was already available we can prevent this leak.
  • Failed to fetch Firebase remote config.. it's kinda similar, fetching is async, and it gets cancelled because it gets unmounted. Checking that it's not unmounted before logging the error, no error.
  • Circular dependencies on native-ui package. They seem to come from components importing from the index of a parent instead of the specific component they rely on, so TimelineItem which is under the Layout folder, importing Flex from ../.. instead of from ../../Flex. No idea if there's a better way but changing the import removed the warning.
  • There's a circular dependency between react-native-crypto -> react-native-randombytes -> sjcl -> react-native-crypto but that's no on our codebase, not sure how to tackle it, any help on this one would be much appreciated.
  • null is not a valid color or brush. I actually looked into this a while ago and remember precisely what it was. It's usage of "null" inside svg files for some of our icons. Luckily I didn't have to spend time because I have good memory. Essentially stroke="null" is invalid yet present in SGB.svg and FTM.svg This gave a warning when listing all currencies, ie add accounts.
  • Bunch of styling warnings in the Portfolio NFTs tab, we should've caught these during review probably. For styled components you need to provide the pxs now. So that should silence the warning.

❓ Context

  • Impacted projects: ledger-live-mobile, ledger-live-desktop, ledger-live-common, native-ui
  • Linked resource(s): ``

βœ… Checklist

  • Test coverage
  • Atomic delivery
  • No breaking changes

πŸ“Έ Demo

No demo

πŸš€ Expectations to reach

App should behave the same as far as QA is concerned, but for developers it should show less warnings.

@live-github-bot
Copy link
Contributor

❌ @juan-cortes

Unfortunately this PR does not comply with the Contributing Conventions and will be closed automatically.

Feel free to reopen this PR once you have browsed through the guidelines.


Found Issues:

  • the branch name chore/nowarnings is invalid

@changeset-bot
Copy link

changeset-bot bot commented Feb 21, 2023

πŸ¦‹ Changeset detected

Latest commit: 764e505

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
live-mobile Patch
@ledgerhq/native-ui Patch
ledger-live-desktop Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@live-github-bot live-github-bot bot closed this Feb 21, 2023
@vercel
Copy link

vercel bot commented Feb 21, 2023

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
live-common-tools πŸ”„ Building (Inspect) Feb 21, 2023 at 11:46PM (UTC)
3 Ignored Deployments
Name Status Preview Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Feb 21, 2023 at 11:46PM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Feb 21, 2023 at 11:46PM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Feb 21, 2023 at 11:46PM (UTC)

@github-actions github-actions bot added common Has changes in live-common desktop Has changes in LLD mobile Has changes in LLM ui Has changes in the design system library labels Feb 21, 2023
@valpinkman valpinkman deleted the chore/nowarnings branch June 23, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD mobile Has changes in LLM ui Has changes in the design system library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant