Conversion to Ketting #9
Conversation
There was a problem hiding this comment.
Figured it would be good to start maintaining a changelog, but happy to delete this if you rather not.
There was a problem hiding this comment.
Agreed. There's a node module that does this automatically, I can look it up.
There was a problem hiding this comment.
I used my own but open to using something else:
There was a problem hiding this comment.
That's cool, let's use yours. We can always harass the author for bug fixes and features. ;)
|
@mattbishop I was gonna adopt eslint from ketting with these settings: https://github.com/badgateway/ketting/blob/main/.eslintrc.json One thing is that they do include and auto-fix semicolons, but I can change the settings to not do that. Let me know if you're down with eslint and if you prefer to take all semi-colons out. I slightly prefer them but definitely not enough to argue about it =) |
|
Forgot to fix the unittests, will take some more time because they rely on Undici's mocking support (which is neat!) but this uses node-fetch under the hood and there's no equivalent =( |
|
I like eslint, as long as we can burn the semicolons with fire! I spent
many years writing C and Java, so it reminds me of where I am to not use
semicolons.
…On Wed, Mar 15, 2023 at 4:57 PM Evert Pot ***@***.***> wrote:
@mattbishop <https://github.com/mattbishop> I was gonna adopt eslint from
ketting with these settings:
https://github.com/badgateway/ketting/blob/main/.eslintrc.json
One thing is that they *do* include and auto-fix semicolons, but I can
change the settings to not do that. Let me know if you're down with eslint
and if you prefer to take all semi-colons out. I slightly prefer them but
definitely not enough to argue about it =)
—
Reply to this email directly, view it on GitHub
<#9 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACY3BLTFSWMXOHUYQG5BRDW4JJNJANCNFSM6AAAAAAV4JQRQU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Alright! Will take some getting used to =) |
|
I'll do that in a later PR so this one doesn't get too noisy. |
|
Hey @mattbishop , I tried to fix the unittests but ran into all kinds of strange issues with types. I suspect that this is because partially the codebase thinks it's working with the new built-in Request and Response classes from fetch, and partially those from I haven't really had issues with this in the past, because they are similar enough. One area they distinctly aren't is how they do streams, which is where all my issues are. So one idea I have is to stop using node-fetch. This is possible if I release Ketting 8 as a beta. I've already made this change here, but I wanted to run this by you in case you believe this may constitute scope creep. Pretty frustrating TBH! You can see also how I've tried to implement mocking functionality with the LMK, so far I've spent about 6 hours on the entire project. |
|
@mattbishop ready for a deeper review, I did all the above and everything now works the way I wanted. I will follow up with another PR that removes all the semicolons (permanently) |
| @@ -15,7 +15,7 @@ | |||
| "@oclif/plugin-help": "5.2.5", | |||
| "@oclif/plugin-not-found": "2.3.20", | |||
| "@oclif/plugin-update": "3.1.5", | |||
| "undici": "5.20.0" | |||
| "ketting": "^8.0.0-alpha.1" | |||
There was a problem hiding this comment.
It's the alpha. Assume that's ok! No more node-fetch yay
| expect(req.headers.get('Authorization')).to.equal(`Bearer ${testToken}`); | ||
| const path = new URL(req.url).pathname; | ||
| switch(path) { | ||
| case '/ledgers/download' : |
There was a problem hiding this comment.
Following links means multiple hops and requests. This is kinda like a mini-router. We can probably make this even better and less verbose but I didn't want to go overboard with this.
There was a problem hiding this comment.
Makes sense. Mocking the links basically.
There was a problem hiding this comment.
Some utilities to make testing HTTP related stuff less painful
| } | ||
| describe("ledger:download", async () => { | ||
|
|
||
| it('should work', () => { |
There was a problem hiding this comment.
Does this setup need to be in an it() ?
There was a problem hiding this comment.
Yes, because everything in describe gets executed at the start of the test suite, to collect the individual test cases.
This makes it very hard to debug individual tests. The real test cases should always be in the it() callbacks.
These days this is also built into node btw so we technically don't need mocha
There was a problem hiding this comment.
Do you think we should convert these to node's test framework? I have never used it so I don't know. I'll create a ticket for you to look into it.
No description provided.