lib: switch to typescript and remove python as a dependency #3
Conversation
There was a problem hiding this comment.
Okay so, this looks pretty good and valuable. There are a few top level things to consider:
- This PR really contains two major changes which might be nice to decouple: the deprecation of the pythonia dep, and the conversion to TS (for example, I would be able to approve a PR for just the work on the former immediately
- TS support will be good, as types are something consumers of this lib will likely want; however, there is a lot more going on now both in terms of abstractions as well as surface area of the deps; my orientation towards this change basically hinges on wether we can expect assistance with maintenance from other teams on an ongoing basis. Standalone, these changes would negatively impact the current maintainers' ability to provide reasonable SLAs on future fixes, imo.
- it seems like the functionality from dwim, enjs, and dejs allow this library to be more concise which is nice. They are a cause for similar concern to Prefer dev dependency in install instructions #2 above, since they are hardly self documenting and I can't find any documentation on how they actually work. Again, if the maintainers of nockjs were more actively involved in this repo than concern is dampened (although I still think docs would be good).
- there might also be room for another library in between nockjs and clack — the design goal there would be that maintainers of the functionality in khan.js and index.js should be able to operate without having to be familiar with nouns on a byte/buffer level. enjs and dejs still seem to be low level enough that this would not be the case.
| } | ||
|
|
||
| // decodes response and unpacks it to a noun, | ||
| // if it has the expected structure of a fyrd response |
There was a problem hiding this comment.
does this handle arbitrarily large trees and convert them to a JSON tree of JS primitives? How? is enjs documented somewhere other than the code?
|
|
||
| // returns a js Noun Object with the structure of a %fyrd | ||
| function Fyrd(desk: string, thread: string, mark_out: string, mark_in: string, atom: string) { | ||
| return dwim([ |
There was a problem hiding this comment.
this is more concise, but it hides the structure of what Fyrd() is constructing. is dwim documented somewhere?
| @@ -0,0 +1,30 @@ | |||
| import { Atom, bigintToDataView, cue_bytes, Noun } from '@urbit/nockjs' | |||
|
|
|||
There was a problem hiding this comment.
Changes in this file are awesome — js buffers are much easier to reason about in TS is seems — thanks for killing this ugly dep.
| } | ||
|
|
||
| export { | ||
| newtDecode, |
There was a problem hiding this comment.
are the two exports that were implicitly removed here made redundant by something in nockjs/enjs? is there docs on that?
There was a problem hiding this comment.
this adds a pretty huge surface area to the software supply chain of this package. I get that a majority I not all will be dev devs, but it seems worth discussing tradeoffs wrt maintenance burden.
There was a problem hiding this comment.
is this abstraction fully made redundant by nockjs? seems like there is room for a layer in-between nockjs and what the khan.js file of this project wants to be. Maybe this should be a separate package or something. open to thoughts.
| return Fyrd('base', 'khan-eval', 'noun', 'ted-eval', hoon); | ||
| } | ||
|
|
||
| interface ThreadResponse { |
There was a problem hiding this comment.
the need for this makes me think we might want to rethink the approach of parseThreadResponse() in a TS context. Maybe its fine as is, maybe this is code smell though.
| { | ||
| nom: 'node', | ||
| get: enjs.frond([ | ||
| { tag: '\x01', get: inner }, |
There was a problem hiding this comment.
yeah, I think there is a missing layer this could be moved down to, ideally devs reading or maintaining this file can be fully insulated from having to think about bytes/hex etc.
There was a problem hiding this comment.
nice additions to the suite. We might also need to add something where the returned value is a larger tree rather than a string, it's hard to intuit that the new parseThreadResponse handles that correctly just by looking at the code. especially without intimate knowledge of dwim, enjs, and dejs.
This switches the repo to using Typescript so that consumers of the library get rich types. It also removes the dependency on python by doing all the operations in JS using the latest @urbit/nockjs to help with encoding/decoding. Didn't want to move lib -> src but dts kinda bakes this assumption in, if y'all have other recommendations I can modify.