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.

Sign up for GitHub

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

Jump to bottom

refactor: Typescript conversion of get-provider-state.js #23635

Merged
merged 18 commits into from
Oct 2, 2024
Merged

refactor: Typescript conversion of get-provider-state.js #23635

merged 18 commits into from
Oct 2, 2024

Conversation

NiranjanaBinoy
Copy link
Contributor

@NiranjanaBinoy NiranjanaBinoy commented Mar 21, 2024 •
edited
Loading

Part of #23014
Fixes #23469

Converting the level 6 dependency file app/scripts/lib/rpc-method-middleware/handlers/get-provider-state.js to typescript for contributing to metamask-controller.js.

Description

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@NiranjanaBinoy NiranjanaBinoy self-assigned this Mar 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

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.

MajorLift
MajorLift reviewed Mar 22, 2024
View reviewed changes
Copy link
Contributor

@MajorLift MajorLift left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if we have any documentation around how to handle jsdoc comments differently in JavaScript vs. TypeScript but we probably should. Some usual suspects:

  • Type annotations with {} syntax are no longer necessary in @param, @property.
  • @type, @typedef tags are no longer necessary (and are not recognized by typedoc).

Having hard-coded types in jsdoc acting as a redundant source of truth makes intellisense and typedoc output brittle and potentially incorrect. It's better to consistently rely on the TypeScript compiler for up-to-date and accurate type information.

app/scripts/lib/rpc-method-middleware/handlers/handlers-helper.ts Outdated Show resolved Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/handlers-helper.ts Outdated Show resolved Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/get-provider-state.ts Outdated Show resolved Hide resolved
app/scripts/lib/rpc-method-middleware/handlers/get-provider-state.ts Outdated Show resolved Hide resolved
@NiranjanaBinoy NiranjanaBinoy changed the base branch from develop to extract-wrapper-type March 27, 2024 21:12
@NiranjanaBinoy NiranjanaBinoy changed the base branch from extract-wrapper-type to develop March 27, 2024 21:12
@NiranjanaBinoy NiranjanaBinoy changed the base branch from develop to extract-wrapper-type March 28, 2024 02:23
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 8, 2024

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added the stale issues and PRs marked as stale label Jun 8, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions.

@github-actions github-actions bot closed this Jun 22, 2024
@github-actions github-actions bot removed the stale issues and PRs marked as stale label Jun 25, 2024
@NiranjanaBinoy NiranjanaBinoy changed the base branch from extract-wrapper-type to develop June 25, 2024 14:55
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 18, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [b273a78]
Page Load Metrics (88 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint684021218139
domContentLoaded1099302512
load41298886933
domInteractive1098302512
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 18 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@codecov Codecov
Copy link

codecov bot commented Jul 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.14%. Comparing base (ce04ae8) to head (49b9bb4).

Files with missing lines Patch % Lines
...c-method-middleware/handlers/get-provider-state.ts 20.00% 4 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #23635      +/-   ##
===========================================
- Coverage    70.14%   70.14%   -0.00%     
===========================================
  Files         1424     1424              
  Lines        49572    49573       +1     
  Branches     13868    13868              
===========================================
  Hits         34769    34769              
- Misses       14803    14804       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NiranjanaBinoy NiranjanaBinoy marked this pull request as ready for review July 18, 2024 20:09
@NiranjanaBinoy NiranjanaBinoy requested a review from a team as a code owner July 18, 2024 20:09
@MajorLift MajorLift mentioned this pull request Aug 20, 2024
Merged
8 tasks
@metamaskbot
Copy link
Collaborator

Builds ready [49b9bb4]
Page Load Metrics (1641 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14882049164211857
domContentLoaded14811979162210852
load14892053164111957
domInteractive137730147
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 11 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [57bbf30]
Page Load Metrics (1726 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25022591665361173
domContentLoaded15092039170611856
load15132145172613866
domInteractive157235136
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

MajorLift
MajorLift reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

Had one question. Otherwise LGTM.

app/scripts/lib/rpc-method-middleware/handlers/get-provider-state.ts Outdated Show resolved Hide resolved
@NiranjanaBinoy NiranjanaBinoy requested a review from a team September 30, 2024 20:36
@metamaskbot
Copy link
Collaborator

Builds ready [268df02]
Page Load Metrics (1922 ± 93 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint40522251771471226
domContentLoaded15982236189118287
load16082286192219493
domInteractive14185493617
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [9a0fc31]
Page Load Metrics (1996 ± 80 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30923561813522251
domContentLoaded16912341197516780
load17042346199616680
domInteractive1992472311
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

MajorLift
MajorLift approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

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

LGTM!

@NiranjanaBinoy NiranjanaBinoy added this pull request to the merge queue Oct 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 2, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 2, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [97c5ce6]
Page Load Metrics (2126 ± 121 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33725941745736353
domContentLoaded176325062098242116
load177325472126251121
domInteractive26163503416
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@NiranjanaBinoy NiranjanaBinoy added this pull request to the merge queue Oct 2, 2024
Merged via the queue into develop with commit a7aacf8 Oct 2, 2024
77 checks passed
@NiranjanaBinoy NiranjanaBinoy deleted the ts-get-provider-state branch October 2, 2024 21:42
@github-actions github-actions bot locked and limited conversation to collaborators Oct 2, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DDDDDanica DDDDDanica DDDDDanica approved these changes

@MajorLift MajorLift MajorLift approved these changes

Assignees

@NiranjanaBinoy NiranjanaBinoy

Labels
release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-extension-platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert get-provider-state.js to Typescript
4 participants
@NiranjanaBinoy @metamaskbot @DDDDDanica @MajorLift