Skip to content

Fix JavaScript static type errors#5879

Closed
leotm wants to merge 5 commits intoMetaMask:mainfrom
leotm:fix/javascript-static-type-errors
Closed

Fix JavaScript static type errors#5879
leotm wants to merge 5 commits intoMetaMask:mainfrom
leotm:fix/javascript-static-type-errors

Conversation

@leotm
Copy link
Copy Markdown
Contributor

@leotm leotm commented Mar 2, 2023 •
edited
Loading

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Fix: #5878

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

cc @Cal-L @gantunesr

@leotm leotm requested a review from a team as a code owner March 2, 2023 11:38
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 2, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@leotm leotm mentioned this pull request Mar 2, 2023
Closed
@gantunesr gantunesr added No QA Needed Apply this label when your PR does not need any QA effort. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 3, 2023
@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 3, 2023 •
edited
Loading

didn't initially see the 2 failing checks, taking a look

nb: and convert more JS files to TS along the way
nb: i'll add labels in future once permissions sorted

TODO

  • fix lint (mostly no-shadows so renames only)
    • husky pre-commit hook should run lint-staged (eslint) and prevent push?
  • fix tests (non-components)

@leotm leotm marked this pull request as draft March 3, 2023 15:00
@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 3, 2023 •
edited
Loading

Hey @leotm, looks like this PR has a lot of linting errors

yep spotted taking a look ^ our husky pre-commit hook running lint-staged is only running eslint not yarn lint, i'll PR too as separate follow-up

@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 3, 2023 •
edited
Loading

as i'm renaming (prefixing, we could also suffix) our ~50ish destructed core>engine controller variables from context

Screenshot 2023-03-03 at 18 21 49

due to duplicate names from upper scope (imports) - not necessarily a bad thing

Screenshot 2023-03-03 at 18 22 57

are these renames something we prefer? as we continue refactoring from JS to TS and restore our type checking (TSC) while fixing type errors...

or happy to disable @typescript-eslint/no-shadow? like our currently disabled no-shadow lint rule (likely originally done for enum's)

some food for thought for Monday i'll bring up then ^

@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 6, 2023 •
edited
Loading

after some thought it's not worth reducing strictness and disabling @typescript-eslint/no-shadow for our entire codebase

so only disabled it for Engine.ts for now until decided whether we want to go ahead with aforementioned mass exodus of var renaming to satisfy this rule as we discover more converting JS files to TS

@leotm leotm marked this pull request as ready for review March 6, 2023 16:06
leotm
leotm commented Mar 6, 2023
View reviewed changes
app/core/Engine.ts
Comment on lines +776 to +778
for (const importedAccount of importedAccounts) {
await KeyringController.importAccountWithStrategy('privateKey', [
importedAccounts[i],
importedAccount,
Copy link
Copy Markdown
Contributor Author

@leotm leotm Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

^ simpler refactor than:

      for (const id of importedAccounts) {
        await KeyringController.importAccountWithStrategy('privateKey', [
          importedAccounts[id],
        ]);
      }

@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 6, 2023 •
edited
Loading

thanks @sethkfman for re-actioning CI ^ fixing the non-components tests then re-readying up (hopefully get contributor permish soon)

edit: goddarn memory leak freezing internet again 😅 getting there with
yarn test:unit:non-components --forceExit --silent

@leotm leotm marked this pull request as draft March 6, 2023 17:10
@leotm leotm marked this pull request as ready for review March 6, 2023 17:45
@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 6, 2023

fixed the cause of the non-component tests in a495187

@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 6, 2023 •
edited
Loading

after fixing the tests, i'm still getting this for 1 test, even on main after fresh git clean -fdx and yarn setup 🤔
any ideas?

Screenshot 2023-03-06 at 17 45 39

yarn jest app/core/Authentication/Authentication.test.ts passing 13/13

but no bueno with yarn test or yarn test:unit:non-components

edit: raised in

@leotm leotm marked this pull request as draft March 16, 2023 09:59
@leotm
Copy link
Copy Markdown
Contributor Author

leotm commented Mar 16, 2023 •
edited
Loading

Add more commits by pushing to the fix/javascript-static-type-errors branch on leotm/metamask-mobile.

Closing in favour of new PR, old fork no longer exists, pushing/publishing no longer working and:

gh pr checkout 5879

From https://github.com/MetaMask/metamask-mobile
! [rejected] refs/pull/5879/head -> fix/javascript-static-type-errors (non-fast-forward)
failed to run git: exit status 1

Edit: New PR with new branch name

@leotm leotm closed this Mar 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2023
@leotm leotm deleted the fix/javascript-static-type-errors branch March 16, 2023 10:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) No QA Needed Apply this label when your PR does not need any QA effort.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JavaScript static type errors

2 participants

@leotm @gantunesr