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

refactor: convert icon-factory.js to typescript #23823

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

davidmurdoch
Copy link
Contributor

@davidmurdoch davidmurdoch commented Apr 1, 2024

No description provided.

Copy link
Contributor

github-actions bot commented Apr 1, 2024

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.

@davidmurdoch davidmurdoch changed the title refactor: convert icon-factory.js to typescript refactor: convert icon-factory.js to typescript Apr 1, 2024
@davidmurdoch davidmurdoch force-pushed the typescript-icon-factory branch from fa2d785 to 8e21b92 Compare April 1, 2024 19:18
Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.99%. Comparing base (e775193) to head (8e21b92).

❗ Current head 8e21b92 differs from pull request most recent head 08b602a. Consider uploading reports for the commit 08b602a to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #23823      +/-   ##
===========================================
+ Coverage    67.36%   68.99%   +1.63%     
===========================================
  Files         1282     1164     -118     
  Lines        50011    44513    -5498     
  Branches     12983    11887    -1096     
===========================================
- Hits         33685    30709    -2976     
+ Misses       16326    13804    -2522     

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

@metamaskbot
Copy link
Collaborator

Builds ready [8e21b92]
Page Load Metrics (1034 ± 508 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint622081214019
domContentLoaded107927178
load50256710341058508
domInteractive107927178
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 452 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@davidmurdoch davidmurdoch marked this pull request as ready for review April 1, 2024 19:57
@davidmurdoch davidmurdoch requested a review from a team as a code owner April 1, 2024 19:57
function iconExistsFor(
address: string,
tokenMetadata?: Partial<TokenMetadata>,
): tokenMetadata is TokenMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be boolean here?

Suggested change
): tokenMetadata is TokenMetadata {
): boolean {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the iconExistsFor function acts as a type guard via a type predicate (the tokenMetadata is TokenMetadata part).

It enables the following, where tokenMetadata is optional in iconExistsFor, but not in imageElFor. TypeScript isn't "smart" enough to infer that tokenMetadata is never null/undefined when iconExistsFor returns true, so a type predicate is used to give TypeScript that information, otherwise we'd have to cast TokenMetadata or check for null/undefined again inside the if statement.

    if (iconExistsFor(address, tokenMetadata)) {
      return imageElFor(tokenMetadata);
    }

See https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates for details.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks ! it makes sense !

@davidmurdoch davidmurdoch requested a review from DDDDDanica May 1, 2024 19:27
@legobeat legobeat force-pushed the typescript-icon-factory branch from 8e21b92 to 08b602a Compare May 7, 2024 11:49
@legobeat legobeat requested a review from a team May 21, 2024 16:05
Copy link
Contributor

@NiranjanaBinoy NiranjanaBinoy left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and I tested them locally.

Copy link

sonarqubecloud bot commented Aug 1, 2024

@HowardBraham HowardBraham merged commit 883fd75 into develop Aug 1, 2024
74 of 75 checks passed
@HowardBraham HowardBraham deleted the typescript-icon-factory branch August 1, 2024 17:39
@github-actions github-actions bot locked and limited conversation to collaborators Aug 1, 2024
@metamaskbot metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 1, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [f4bfed4]
Page Load Metrics (457 ± 372 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint784261429847
domContentLoaded1078282210
load442115457776372
domInteractive1078282210
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 452 Bytes (0.01%)

@gauthierpetetin gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants