Page MenuHomePhabricator

[Cashtab] Convert some common components to ts
ClosedPublic

Authored by bytesofman on Thu, Dec 26, 18:21.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC602ebcdf598a: [Cashtab] Convert some common components to ts
Summary

Part of ongoing effort to convert Cashtab to ts

Convert some small Common components to ts

Decision to include these particular components is arbitrary. Felt like doing just one was too small a diff. Adding any more was too much for one diff.

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Tail of the build log:

> ecash-lib@1.2.0 build
> tsc && tsc -p ./tsconfig.build.json && cp -r ./src/ffi ./dist

Installing ecash-agora dependencies...
/work/modules/ecash-agora /work/modules/ecash-lib /work/modules/b58-ts /work/modules/ecash-lib-wasm /work/modules/ecash-script /work/modules/mock-chronik-client /work/modules/chronik-client /work/modules/ecashaddrjs /work/abc-ci-builds/cashtab-tests

added 364 packages, and audited 367 packages in 1s

60 packages are looking for funding
  run `npm fund` for details

2 vulnerabilities (1 moderate, 1 high)

To address all issues, run:
  npm audit fix

Run `npm audit` for details.

> ecash-agora@0.2.0 build
> tsc && tsc -p ./tsconfig.build.json

/work/cashtab /work/modules/ecash-agora /work/modules/ecash-lib /work/modules/b58-ts /work/modules/ecash-lib-wasm /work/modules/ecash-script /work/modules/mock-chronik-client /work/modules/chronik-client /work/modules/ecashaddrjs /work/abc-ci-builds/cashtab-tests
npm warn deprecated @humanwhocodes/config-array@0.11.14: Use @eslint/config-array instead
npm warn deprecated @humanwhocodes/object-schema@2.0.3: Use @eslint/object-schema instead
npm warn deprecated eslint@8.56.0: This version is no longer supported. Please see https://eslint.org/version-support for other options.

added 1488 packages, and audited 3309 packages in 24s

320 packages are looking for funding
  run `npm fund` for details

8 vulnerabilities (6 moderate, 2 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues (including breaking changes), run:
  npm audit fix --force

Run `npm audit` for details.

> cashtab@3.4.5 build
> node scripts/build.js

Creating an optimized production build...
Failed to compile.

TS2322: Type 'string | null' is not assignable to type 'string'.
  Type 'null' is not assignable to type 'string'.
    2240 |                     ) : (
    2241 |                         <BalanceHeaderToken
  > 2242 |                             formattedDecimalizedTokenBalance={
         |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    2243 |                                 typeof tokenBalance === 'string'
    2244 |                                     ? decimalizedTokenQtyToLocaleFormat(
    2245 |                                           tokenBalance,


Build cashtab-tests failed with exit code 1
Fabien added inline comments.
cashtab/src/components/Common/AvalancheFinalized.tsx
37 ↗(On Diff #51753)

where is title ?

bytesofman marked an inline comment as done.

comment clarifying strange typescript/styled-components interaction

cashtab/src/components/Common/AvalancheFinalized.tsx
37 ↗(On Diff #51753)

This one is weird and I think has to do with how the styled-components library is interacting with TypeScript and svg elements specifically.

The reason I have added title here is because, if I do not, I get a lint error on the line title="Finalized by Avalanche" below

In that line, I am hardcoding a title for this SVG element -- since this is what this SVG means.

But for whatever reason, styled-components does not recognize that Title is a native attribute of the svg element. It thinks I am specifiying an undefined prop.

So, on the one hand, maybe it is better to suppress the lint error or use some kind of warning. On the other hand, it is true that the Finalized component does have a title prop (even though we do not actually use it in the styled component).

I think this is the cleanest solution though yes, any future dev would be confused. Added a comment to explain this situation.

This revision is now accepted and ready to land.Fri, Dec 27, 15:26