Details

Reviewers

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

Branch

ecashaddrjs-to-ts

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30245
Build 60017: Build Diff	ecashaddrjs-tests
Build 60016: arc lint + arc unit

Event Timeline

bytesofman created this revision.Sep 10 2024, 23:40

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 10 2024, 23:40

Harbormaster completed remote builds in B30241: Diff 49575.Sep 10 2024, 23:41

version bump, changelog, less confusing validation implementation

Harbormaster completed remote builds in B30242: Diff 49576.Sep 10 2024, 23:47

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

Harbormaster completed remote builds in B30243: Diff 49577.Sep 11 2024, 02:38

preserve legacy import syntax

Harbormaster completed remote builds in B30244: Diff 49578.Sep 11 2024, 03:06

remove unused dependency

Harbormaster completed remote builds in B30245: Diff 49579.Sep 11 2024, 03:12

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

teamcity awarded a token.Sep 11 2024, 03:14

Harbormaster completed remote builds in B30246: Diff 49580.Sep 11 2024, 03:15

bytesofman published this revision for review.Sep 11 2024, 03:24

emack requested changes to this revision.Sep 11 2024, 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.Sep 11 2024, 05:55

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.

emack accepted this revision.Sep 11 2024, 06:32

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

@bot ecash-herald-tests

teamcity updated a token.Sep 11 2024, 13:03

Closed by commit rABC91adf60e93c8: [ecashaddrjs] Convert to typescript (authored by bytesofman). · Explain WhySep 11 2024, 13:37

This revision was automatically updated to reflect the committed changes.

bytesofman added a commit: rABC91adf60e93c8: [ecashaddrjs] Convert to typescript.

tobias_ruck added a subscriber: tobias_ruck.Sep 11 2024, 13:38

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 marked 6 inline comments as done.Sep 11 2024, 13:45

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

[ecashaddrjs] Convert to typescript
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 49579

contrib/teamcity/build-configurations.yml

modules/ecashaddrjs/.mocharc.js

modules/ecashaddrjs/README.md

modules/ecashaddrjs/package-lock.json

modules/ecashaddrjs/package.json

modules/ecashaddrjs/src/base32.js

modules/ecashaddrjs/src/base32.ts

modules/ecashaddrjs/src/cashaddr.d.ts

modules/ecashaddrjs/src/cashaddr.js

modules/ecashaddrjs/src/cashaddr.ts

modules/ecashaddrjs/src/convertBits.js

modules/ecashaddrjs/src/convertBits.ts

modules/ecashaddrjs/src/types.ts

modules/ecashaddrjs/src/validation.js

modules/ecashaddrjs/src/validation.ts

modules/ecashaddrjs/test/base32.js

modules/ecashaddrjs/test/base32.test.ts

modules/ecashaddrjs/test/cashaddr.js

modules/ecashaddrjs/test/cashaddr.test.ts

modules/ecashaddrjs/test/convertBits.js

modules/ecashaddrjs/test/convertBits.ts

modules/ecashaddrjs/tsconfig.json

modules/ecashaddrjs/webpack.config.js

[ecashaddrjs] Convert to typescriptClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 49579

contrib/teamcity/build-configurations.yml

modules/ecashaddrjs/.mocharc.js

modules/ecashaddrjs/README.md

modules/ecashaddrjs/package-lock.json

modules/ecashaddrjs/package.json

modules/ecashaddrjs/src/base32.js

modules/ecashaddrjs/src/base32.ts

modules/ecashaddrjs/src/cashaddr.d.ts

modules/ecashaddrjs/src/cashaddr.js

modules/ecashaddrjs/src/cashaddr.ts

modules/ecashaddrjs/src/convertBits.js

modules/ecashaddrjs/src/convertBits.ts

modules/ecashaddrjs/src/types.ts

modules/ecashaddrjs/src/validation.js

modules/ecashaddrjs/src/validation.ts

modules/ecashaddrjs/test/base32.js

modules/ecashaddrjs/test/base32.test.ts

modules/ecashaddrjs/test/cashaddr.js

modules/ecashaddrjs/test/cashaddr.test.ts

modules/ecashaddrjs/test/convertBits.js

modules/ecashaddrjs/test/convertBits.ts

modules/ecashaddrjs/tsconfig.json

modules/ecashaddrjs/webpack.config.js

[ecashaddrjs] Convert to typescript
ClosedPublic
Actions

Revision Contents
Changeset List