Improving types, readme & tests #85

corollari · 2020-09-02T19:43:32Z

This PR will implement the changes discussed on discord:

Convert readme to markdown on CI
Generate typescript declarations
Initial work on improving tests

And adds a few extra fixes:

Fixes the example code on the readme (it was outdated)
Adds a type that was forgotten
Bumps some dependencies

Also, should I split this into multiple PRs? I thought that given that the changes made here are quite small a single PR might work best.

TS declarations

Once this (or the commit for generating ts declarations) gets merged and published as a package I'll also deprecate the DefinitelyTyped package, as it will be useless at this point.

A note on tests

On discord I mentioned that I'd start working on better tests that use a real environment, however now I think that it's best to focus my time on ln2tbtc (the submarine swaps project) to push it to mainnet. In any case, I've dusted off the previous tests and got them working again, but if you feel that they just don't add much value feel free to remove that commit.

corollari · 2020-09-02T22:02:54Z

Also, currently the typechecks done on the CI are broken, I've tried to replace the jq script with a --project flag that points to jsconfig.json but that hasn't worked (I don't know why tho)

corollari · 2020-09-10T19:07:51Z

Fixed merge conflicts

corollari · 2020-09-10T19:18:56Z

Note: f2da1ab is also applied on #91

Shadowfiend

Sorry for the long wait here---had a look and left some notes. This is looking awesome overall though, I think when we merge it I'll run a stable release so we have types in the main package.

Shadowfiend · 2020-09-22T14:16:29Z

.github/workflows/node.yml

+    - name: Install pandoc
+      run: wget https://github.com/jgm/pandoc/releases/download/2.10.1/pandoc-2.10.1-1-amd64.deb && sudo dpkg -i pandoc-2.10.1-1-amd64.deb
+    - name: Convert readme to markdown
+      run: asciidoctor README.adoc -b docbook -o - | pandoc -f docbook -t gfm > README.md


I experimented with this a bit the other day---I think the move here is:

asciidoctor README.adoc -o - | pandoc -f html -t markdown_strict - -o README.md

Going through HTML seems to produce better Markdown output (e.g., it includes the Table of Contents and title) than using Docbook as the intermediary.

Ah I see, that's probably better than my command since I didn't directly compare the results achieved when using different intermediary formats.

Shadowfiend · 2020-09-22T14:16:49Z

README.adoc

-        }
+        server: "electrumx-server.test.tbtc.network",
+        port: 8443,
+        protocol: "wss"


Shadowfiend · 2020-09-22T14:17:27Z

package.json

  "scripts": {
    "test": "mocha --timeout 10000",
    "lint": "npm run lint:js",
    "lint:fix": "npm run lint:fix:js",
    "lint:js": "npm run lint:js:eslint && npm run lint:js:types",
    "lint:fix:js": "eslint --fix .",
    "lint:js:eslint": "eslint .",
-    "lint:js:types": "!(npx tsc --allowJs --noEmit $(cat jsconfig.json | jq -r '.compilerOptions | to_entries | map([\"--\\(.key)\",.value]) | flatten | join(\" \")') src/**.js bin/**.js examples/**.js | grep \"^\\(src\\|bin\\|test\\|examples\\)/\") # comment any other passed arguments"
+    "lint:js:types": "!(npx tsc --allowJs --noEmit $(cat jsconfig.json | jq -r '.compilerOptions | to_entries | map([\"--\\(.key)\",.value]) | flatten | join(\" \")') src/**.js bin/**.js examples/**.js | grep \"^\\(src\\|bin\\|test\\|examples\\)/\") # comment any other passed arguments",


Just to confirm here---ultimately this did end up working?

No, I spent several hours trying to get it to work through other methods that don't involve having to parse the .json file directly (would solve the problem on CI while simplifying the command heavily) but I couldn't get it to work, so it's still broken on CI runners.

Probably the easiest way to solve that problem would be patching/upgrading the version of jq used on CI, as that is what causes the breakage. If you feel that's a good solution I'll bundle it into this PR.

Let's actually pull it to a new PR if that's okay---this one's getting big and starting to drift from coherence haha.

I spent several hours trying to get it to work through other methods that don't involve having to parse the .json file directly

Side note: I did this when I originally landed on this solution too haha. It's brutal.

Let's actually pull it to a new PR if that's okay

On it!

Side note: I did this when I originally landed on this solution too haha. It's brutal.

Yeah, I spent way too much time trying to get it to work with tsc's --project, which seems to be made for this type of use-cases, but while the options were correctly parsed I kept running into problems 😖

Shadowfiend · 2020-09-22T14:17:38Z

package.json

  "scripts": {
    "test": "mocha --timeout 10000",
    "lint": "npm run lint:js",
    "lint:fix": "npm run lint:fix:js",
    "lint:js": "npm run lint:js:eslint && npm run lint:js:types",
    "lint:fix:js": "eslint --fix .",
    "lint:js:eslint": "eslint .",
-    "lint:js:types": "!(npx tsc --allowJs --noEmit $(cat jsconfig.json | jq -r '.compilerOptions | to_entries | map([\"--\\(.key)\",.value]) | flatten | join(\" \")') src/**.js bin/**.js examples/**.js | grep \"^\\(src\\|bin\\|test\\|examples\\)/\") # comment any other passed arguments"
+    "lint:js:types": "!(npx tsc --allowJs --noEmit $(cat jsconfig.json | jq -r '.compilerOptions | to_entries | map([\"--\\(.key)\",.value]) | flatten | join(\" \")') src/**.js bin/**.js examples/**.js | grep \"^\\(src\\|bin\\|test\\|examples\\)/\") # comment any other passed arguments",
+    "generate:types": "tsc src/*.js --declaration --allowJs --emitDeclarationOnly --outDir types"


Shadowfiend · 2020-09-22T14:18:00Z

src/lib/ElectrumClient.js

@@ -1,6 +1,6 @@
 import ElectrumClient from "electrum-client-js"
 import sha256 from "bcrypto/lib/sha256.js"
-import { backoffRetrier } from "./backoff"
+import { backoffRetrier } from "./backoff.js"


Shadowfiend · 2020-09-22T14:19:22Z

test/BitcoinSPVTest.js

+import { BitcoinSPV } from "../src/lib/BitcoinSPV.js"
+import ElectrumClient from "../src/lib/ElectrumClient.js"
+import fs from "fs"
+import chai from "chai"


We generally import these at the global level rather than invoking them on chai. Is the reason here to satisfy TypeScript? I seem to recall having run into an issue or two with TypeScript, chai, and tests, but remember figuring out how to deal with it… Knowledge that is lost but likely easily found hehe.

To be honest I don't really remember the exact reason why I went with this. I'll look into doing it in other ways 👌

Shadowfiend · 2020-09-22T14:20:52Z

src/lib/ElectrumClient.js

@@ -467,7 +467,7 @@ export default class Client {
 * @param {string} script ScriptPubKey in a hexadecimal format.
 * @return {string} Script hash as a hex string.
 */
-function scriptToHash(script) {
+export function scriptToHash(script) {


Wish there was a way to export this for testing without exporting it for everyone else 🤔

I thought of using something like rewire but given that this is an internal file which is not meant to be used as part of the library I thought it would be okay to just export it.

What do you think would be the best solution among these?

Move it into it's own file and test that directly

Spy on the implementation using rewire

Sorry, I should have been clearer---I'm fine with it as is, don't think we need to do any additional work right now. I've been thinking about it some more and really, it might even make more sense as a function of ElectrumClient, which is where it was in the first place. Either way, I don't think it needs any more work right now, we are past Good Enough™.

Got it! 👌

corollari marked this pull request as ready for review September 3, 2020 09:02

corollari force-pushed the qol-improvements branch from 25fccd8 to 4675872 Compare September 3, 2020 09:14

corollari changed the title ~~A few improvements~~ Improving types, readme & tests Sep 3, 2020

corollari added 7 commits September 10, 2020 21:06

Generate markdown readme on CI

cc43ea9

Add type declaration generation

cf60a6a

Update package-lock

3d02097

Fix example code on readme

9008ac0

Update mocha

33e946a

Update tests

d0141f0

Add type generation to CI

7936f15

corollari force-pushed the qol-improvements branch from e11990b to 7936f15 Compare September 10, 2020 19:07

corollari added 2 commits September 10, 2020 21:15

Bump node version (ESM imports don't work on 12.11)

97ca109

Fix import statement

f2da1ab

Shadowfiend reviewed Sep 22, 2020

View reviewed changes

corollari mentioned this pull request Sep 23, 2020

Upgrade jq on CI runner #95

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improving types, readme & tests #85

Improving types, readme & tests #85

corollari commented Sep 2, 2020 •

edited

Loading

corollari commented Sep 2, 2020 •

edited

Loading

corollari commented Sep 10, 2020

corollari commented Sep 10, 2020

Shadowfiend left a comment

Shadowfiend Sep 22, 2020

corollari Sep 23, 2020

Shadowfiend Sep 22, 2020

Shadowfiend Sep 22, 2020

corollari Sep 23, 2020

Shadowfiend Sep 23, 2020

Shadowfiend Sep 23, 2020

corollari Sep 23, 2020

Shadowfiend Sep 22, 2020

Shadowfiend Sep 22, 2020

Shadowfiend Sep 22, 2020

corollari Sep 23, 2020

Shadowfiend Sep 22, 2020

corollari Sep 23, 2020

Shadowfiend Sep 23, 2020

corollari Sep 23, 2020

Improving types, readme & tests #85

Are you sure you want to change the base?

Improving types, readme & tests #85

Conversation

corollari commented Sep 2, 2020 • edited Loading

TS declarations

A note on tests

corollari commented Sep 2, 2020 • edited Loading

corollari commented Sep 10, 2020

corollari commented Sep 10, 2020

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corollari commented Sep 2, 2020 •

edited

Loading

corollari commented Sep 2, 2020 •

edited

Loading