Page MenuHomePhabricator

[ecashaddrjs] Convert to typescript
ClosedPublic

Authored by bytesofman on Tue, Sep 10, 23:40.

Details

Reviewers
emack
Group Reviewers
Restricted Project
Commits
rABC91adf60e93c8: [ecashaddrjs] Convert to typescript
Summary

Typescript is "implemented" in ecashaddrjs by a script-generated types file that at least allows typescript apps to use the module without throwing errors.

However, have been running into some issues that are potentially caused by this implementation in the chronik docs website.

With increasingly complex types coming into XEC app development through ecash-lib, ecash-agora, and chronik-client, keeping typescript out of ecash modules is much more difficult than putting it in. Will need to get into Cashtab soon as well.

So, even if this does not fix the docusaurus issue, we should just implement typescript here anyway.

Test Plan

npm test

Diff Detail

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

Event Timeline

version bump, changelog, less confusing validation implementation

changing import syntax in ecash-lib to build without ts errors

preserve legacy import syntax

run mock-chronik-client tests on module updates

@bot cashtab-tests alias-server-tests ecash-herald-tests token-server-tests chronik-client-tests ecash-lib-tests ecash-agora-integration-tests ecash-lib-integration-tests mock-chronik-client-tests chronik-client-integration-tests

emack requested changes to this revision.Wed, Sep 11, 05:55
emack added a subscriber: emack.
emack added inline comments.
modules/ecashaddrjs/src/base32.ts
88 ↗(On Diff #49580)

it's not custom for typescript comments to include the returns element for completeness?

modules/ecashaddrjs/src/cashaddr.ts
156 ↗(On Diff #49580)
modules/ecashaddrjs/src/types.ts
1 ↗(On Diff #49580)

can't we just lowercase it before parsing instead of listing out upper and lower cap permutations?

This revision now requires changes to proceed.Wed, Sep 11, 05:55
bytesofman marked 3 inline comments as done.

not sure if I called the bot correctly for these tests but the main thing we want to confirm is that none of the libs break

even with that, releasing this as 2.0.0 is something to consider

modules/ecashaddrjs/src/base32.ts
88 ↗(On Diff #49580)

no point really since the typescript syntax shows the expected return type, ie below with

function decode(string: string): Uint8Array {

for consistency, removing stuff in these comments that is explicitly in the typescript syntax of the function defn

modules/ecashaddrjs/src/cashaddr.ts
156 ↗(On Diff #49580)

this is a better comment but imo slightly out of scope for the diff

modules/ecashaddrjs/src/types.ts
1 ↗(On Diff #49580)

should prob do some more refactoring here, but this is the minimum for getting "same functionality, but with typescript"

history here is that this lib used to only support uppercase. then chronik only supported lowercase. so I added the chronikReady param to some functions to modify returned types...but also preserved the legacy stuff.

this legacy support is kind of tricky now, since it is not the default, but going fwd should be. but that fix should happen in another diff.

in this diff i am technically deprecating the previously-supported "mixed case" inputs e.g. "P2pKh" (well, at least for typescript users)... but that was always just something that worked accidentally. I don't think anyone seriously liked this functionality.

This revision is now accepted and ready to land.Wed, Sep 11, 06:32

@bot ecash-herald-tests

This revision was automatically updated to reflect the committed changes.
tobias_ruck added inline comments.
modules/ecashaddrjs/src/base32.ts
12–13 ↗(On Diff #49580)

why not this?

73 ↗(On Diff #49580)

just a suggestion; more in line with the other ts stuff in the repo (see also suggestion at the end of file)

modules/ecashaddrjs/src/cashaddr.ts
14 ↗(On Diff #49580)

any reason not to do this?

570 ↗(On Diff #49580)

same here; might be better to just attach "export" to each function, unless there's a good reason to have a sort-of "summary" here

modules/ecashaddrjs/src/convertBits.ts
25–26 ↗(On Diff #49580)
modules/ecashaddrjs/src/types.ts
1 ↗(On Diff #49580)

missing copyright

bytesofman added inline comments.
modules/ecashaddrjs/src/base32.ts
12–13 ↗(On Diff #49580)

this does look better but in practice I get ts errors from it

Module '"./validation"' has no exported member 'validate'. Did you mean to use 'import validate from "./validation"' instead?ts(2614)
73 ↗(On Diff #49580)

imo this is better, however I think it is important to preserve the legacy API -- do not want to make users change how they import ecashaddrjs into their apps.

may need to overhaul this in a 2.0.0 going forward, it is pretty cumbersome to preserve this API

modules/ecashaddrjs/src/cashaddr.ts
14 ↗(On Diff #49580)

unexpected ts error

570 ↗(On Diff #49580)

yes, this is indeed a weird way to do the export. may need to improve it. for now, the reason it is here is so users do not have to change how they import ecashaddrjs

modules/ecashaddrjs/src/convertBits.ts
25–26 ↗(On Diff #49580)

see above...I don't totally get why this doesn't work, but ts does not like it