Unexpected ts migration for LLM #1000
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 4d0a7aa The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
juan-cortes
force-pushed
the
support/LIVE-3405
branch
from
d952f60
to
494413e
Compare
|
you're not a pawn, you're the chess player, I'm just a spectator making comments hahaha |
Codecov Report
@@ Coverage Diff @@
## develop #1000 +/- ##
========================================
Coverage 47.49% 47.49%
========================================
Files 640 640
Lines 28678 28678
Branches 7387 7387
========================================
Hits 13622 13622
Misses 14998 14998
Partials 58 58
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. |
Screenshots: ❌
It seems this PR contains screenshots that are different from the base branch.
windows
|
juan-cortes
changed the title
juan-cortes
changed the title
|
Come on CI, locally there are no errors, don't ruin my sunday. |
| "live-mobile": minor | ||
| --- | ||
|
|
||
| Migration from JavaScript to TypeScript for LLM |
There was a problem hiding this comment.
I still don't think we need to bump the version of mobile for a refactor
| @@ -5,15 +5,22 @@ module.exports = { | |||
| "airbnb", | |||
| "prettier", | |||
There was a problem hiding this comment.
Should we update prettier to use the same proper config ? https://github.com/prettier/eslint-plugin-prettier#recommended-configuration
| "prettier", | |
| "plugin:prettier/recommended", |
| @@ -5,15 +5,22 @@ module.exports = { | |||
| "airbnb", | |||
| "prettier", | |||
| "plugin:json/recommended", | |||
| "plugin:@typescript-eslint/recommended", | |||
There was a problem hiding this comment.
We can add the recommended rules for the import plugin: https://github.com/import-js/eslint-plugin-import#typescript
| "plugin:@typescript-eslint/recommended", | |
| "plugin:@typescript-eslint/recommended", | |
| "plugin:import/recommended", | |
| "plugin:import/typescript", |
We can probably also add the typed rules for typescript: https://typescript-eslint.io/docs/linting/typed-linting/
| }, | ||
| }, | ||
| plugins: ["prettier", "detox"], | ||
| plugins: ["prettier", "detox", "@typescript-eslint", "import"], |
There was a problem hiding this comment.
This should not be needed when using the recommended rules config for @typescript-eslint and import plugins (and even prettier if we use the correct config):
- https://github.com/typescript-eslint/typescript-eslint/tree/main/packages/eslint-plugin#usage
- https://github.com/import-js/eslint-plugin-import#typescript
- https://github.com/prettier/eslint-plugin-prettier#recommended-configuration
| plugins: ["prettier", "detox", "@typescript-eslint", "import"], | |
| plugins: ["detox"], |
This turned into an unexpected typescript migration and I don't know exactly how it happened.
📝 Description
I promise it was not my intention to hijack the purpose of #991 but it ended up being basically what this is. Following the proposed approach from @LFBarreto over there I went over all the files in the repo, and completed a migration. We have a bunch of house cleaning to do after this though, we need to go over all the warnings, all the unused files, etc. Left more data for this over on slack but it's not yet published so I can't link there just yet.
❓ Context
ledger-live-mobilehttps://ledgerhq.atlassian.net/browse/LIVE-2505✅ Checklist
📸 Demo
No demo needed.
🚀 Expectations to reach
Nothing to test if CI is happy.