Let's Fix TS & Eslint #1141
Merged
Let's Fix TS & Eslint #1141
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Changeset detectedLatest commit: b26fd81 The changes in this PR will be included in the next version bump. This PR includes changesets to release 65 packages
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 |
3 tasks
Codecov ReportBase: 47.94% // Head: 47.95% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1141 +/- ##
========================================
Coverage 47.94% 47.95%
========================================
Files 691 691
Lines 30445 30456 +11
Branches 7977 7989 +12
========================================
+ Hits 14597 14605 +8
- Misses 14636 14637 +1
- Partials 1212 1214 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
Screenshots: β
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
valpinkman
force-pushed
the
support/fix-llm-ts
branch
from
6085c1e
to
2f8a1aa
Compare
github-actions
bot
added
the
ledgerjs
valpinkman
force-pushed
the
support/fix-llm-ts
branch
from
9d4e962
to
77f68c3
Compare
valpinkman
force-pushed
the
support/fix-llm-ts
branch
from
5adc046
to
fb3095f
Compare
valpinkman
force-pushed
the
support/fix-llm-ts
branch
from
fb3095f
to
f3b4d65
Compare
β¦rt is indeed unused)
- Created a Asset type and used it instead of recreating it every time - Remove navigationParams prop in AssetRow as there was a typescript error and it was unused anyway
also remove some unneeded `params: undefined`
valpinkman
force-pushed
the
support/fix-llm-ts
branch
from
05f98ae
to
b26fd81
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
π Description
β What this PR is about
On Ledger Live Mobile:
anyexplicitely is an error, not a warning.π« What this PR is NOT about
This work should be done by other teams (on the parts owned by them) later on.
There are multiple places in LLM where we seem to write code or types in a wrong - or weird - way.
We have put
// FIXMEcomments there that will have to be checked by the relevant teams in the future.We also took some notes about these parts while typing the code: https://github.com/LedgerHQ/ledger-live-issues/issues/32#issuecomment-1246467565
We wrote some documentation on how to type certain parts of the app here https://github.com/LedgerHQ/ledger-live/wiki/LLM:Typescript
π Documentation
We also wrote some guides in the repository wiki about how to properly type some code parts.
β Context
We have already merged the work from #1054
LLM+ 64 others - see changesetsβ Checklist
Maybe breaking changes... we have to carefully test this PR as some adjustment have been made while fixing the errors.
πΈ Demo
Before
After
π Expectations to reach
Zero TS errors, strict mode enabled. Clean slate on LLM typechecker
Pull Requests must pass the CI and be internally validated in order to be merged.
Bonus Round => All the WTF we found (also linked in the comment of the issue)
During our TS rework, we spotted some parts of the app that should definitely be reviewed/refactored:
src/helpers/formatAccountSearchResults.ts=> rework without lodashreduceandforeach, and make sure the return type means something.react-native-redash=> used only twice, we are using v1 (if reanimated version < 2) yet we are using reanimated v2.8.0useThemefrom@react-navigation/nativewhile we are supposed to have migrated to v3 using@ledgerhq/ui, thus the migration to v3 is not finished@ledgerhq/types-liveis broken as it only declares a string. There is anotherDerivationModeinlive-commonthat is correctly typed, but we cannot import it intotypes-liveas that would break the dependency cycle. A refacto/rework is needed to migrate this type totypes-liveinvarianton application side (LLM). should we remove ?/src/components/DeviceJob/index.tsx-> very weird types here, bunch of any and confusion on the type ofmetawhich is supposed to be a Device not a Recordobjectas a type), however I do believe a deeper understanding of the props we pass is necessary and an overview of this system is required{ value: something | null | undefined }.{ value?: something | null }is better, but those null checks are a pain in the ass. It should either be something or undefined