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

Convert shared/modules/network.utils to TS #18352

Merged
merged 4 commits into from
Mar 31, 2023
Merged

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Mar 28, 2023

Explanation

We want to convert NetworkController to TypeScript in order to be able to compare differences in the controller between in this repo and the core repo. To do this, however, we need to convert the dependencies of the controller to TypeScript.

As a part of this effort, this commit converts
shared/modules/network.utils.js to TypeScript, and also adds tests.

Progresses #18285.

Manual Testing Steps

This commit should not introduce any functional changes. All tests should pass, and all functionality in the extension should continue to work.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@mcmire mcmire requested a review from a team as a code owner March 28, 2023 17:56
@mcmire mcmire requested a review from NidhiKJha March 28, 2023 17:56
@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.

We want to convert NetworkController to TypeScript in order to be able
to compare differences in the controller between in this repo and the
core repo. To do this, however, we need to convert the dependencies of
the controller to TypeScript.

As a part of this effort, this commit converts
`shared/modules/network.utils.js` to TypeScript, and also adds tests.
@mcmire mcmire force-pushed the convert-network-utils branch from 157eaf9 to 85cb577 Compare March 28, 2023 17:59
@mcmire mcmire linked an issue Mar 28, 2023 that may be closed by this pull request
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Looks great! The additional unit tests are appreciated. I noticed one minor problem with at test, but otherwise I can approve this when CI is passing

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #18352 (23efe01) into develop (74b9ab2) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #18352      +/-   ##
===========================================
+ Coverage    64.78%   64.80%   +0.02%     
===========================================
  Files          927      927              
  Lines        35698    35684      -14     
  Branches      9164     9158       -6     
===========================================
- Hits         23126    23123       -3     
+ Misses       12572    12561      -11     
Impacted Files Coverage Δ
shared/modules/network.utils.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@metamaskbot
Copy link
Collaborator

Builds ready [4931a90]
Page Load Metrics (1554 ± 55 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint892931184220
domContentLoaded1412180515329747
load14121805155411455
domInteractive1412180515329747
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: 0 bytes
  • common: 10 bytes

Co-authored-by: Mark Stacey <[email protected]>
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [37da849]
Page Load Metrics (2071 ± 115 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1034501968239
domContentLoaded152525032058244117
load152525032071239115
domInteractive152525032058244117
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: 0 bytes
  • common: 10 bytes

@metamaskbot
Copy link
Collaborator

Builds ready [23efe01]
Page Load Metrics (1750 ± 74 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint98160131189
domContentLoaded14442120174515976
load14442120175015574
domInteractive14442120174415976
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 bytes
  • ui: 0 bytes
  • common: 10 bytes

@mcmire mcmire merged commit ae08035 into develop Mar 31, 2023
@mcmire mcmire deleted the convert-network-utils branch March 31, 2023 16:52
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert NetworkController dependencies to TypeScript
4 participants