Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: TypeScript Definitions for core libraries #79

Merged
merged 6 commits into from
Jun 9, 2021

Conversation

rvagg
Copy link
Contributor

@rvagg rvagg commented Mar 15, 2021

This is current work, has been going on for ~1 year now, and is not quite complete. @achingbrain and others have been hard at work trying to wrap it up.

I'm turning it into a project proposal because it has actual high value and high leverage and is a legitimate project that we would very likely prioritise if it wasn't already mostly done. Sudo currently owns this implicitly because it's transitional work, but I'm making it explicit with a project proposal and we're going to properly wrap it up.

@achingbrain, @hugomrdias, @Gozala, @vasco-santos, @jacobheun I'd love some input here to define the right amount of work to complete this in a way that captures as much value as we can and doesn't leave awkward dangling pieces. Are there additional libraries with non-trivial use that are overlooked with the primary focus on js-ipfs dependencies? Is there any additional work needed on tooling (aegir, GitHub Actions, other) that we should have on the list?

Of course I want to keep the scope limited but don't want this to get brushed under the rug because we have real current users who get real value out of this and I think we all expect this work would make it easier to get more users and to increase their utility of our libraries. So let's do it properly, and make sure we can celebrate and get proper acknowledgement for everyone's effort when it's done.

Aside from completing the remaining js-ipfs itegration work, the scope of this project includes some additional libraries that re not currently part of the js-ipfs dependency tree, including:

* Next-generation IPLD codec libraries (using the js-multiformats pattern)
* _TODO: what other non-archived, non-dormant project should we include here to achieve the above value & impact ideals?_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tooling is mentioned above, but we should clarify what we aim to land as part of this. Quite a bit of work has gone into Aegir for type generation, what's the short term milestone/feature set we're targeting for that? Is it already done?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested in input on this, maybe from @hugomrdias or @achingbrain? The aim is something like "get it done properly" but also "not too large in scope that it goes beyond a S/M project". So I don't want to blow this out with tooling investment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding types aegir is pretty much done.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theirs another angle regarding types, which is documentation. Aegir uses typedoc and it does a pretty good job at taking our types and generating documentation.
But because our code base is still commonjs there is still some minor issues. To overcome these issues i normally need to either change the current commonjs exports to something more friendly or file issues in Typedoc to support all the quirks about commonjs exports :)
This may be out of scope but still, up to date documentation is important!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hugomrdias 👍 my take is that the tooling is not mature enough for the annotated-JS case to do a consistently good job of it--it doesn't seem to be a priority on the typedoc end and doing it from the jsdoc end (as I've been trying to do) just doesn't work the more you lean in to TS (nobody seems to be interested in unifying the two). But this is certainly something we should take on in a near-future project, to build better and more consistent docs across our stack with tight integration with the TS annotations.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where exactly to fit this and I believe it is out of scope for this specific proposal, but thought I'd still call this out here. We leverage TS a bit across message-port-client / server pieces to ensure that both ends comply with a wire protocol, I think it could be generalized a bit more to provide same value across more places where we hav client / server or other RPC model.

proposals/79-typescript-definitions.md Outdated Show resolved Hide resolved
proposals/79-typescript-definitions.md Outdated Show resolved Hide resolved

Aside from completing the remaining js-ipfs itegration work, the scope of this project includes some additional libraries that re not currently part of the js-ipfs dependency tree, including:

* Next-generation IPLD codec libraries (using the js-multiformats pattern)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this include multiaddr update? multiformats/js-multiaddr#159

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it probably should be, but I don't have a feel for how much work it is to get that over the line. Do you?

Copy link
Contributor

@vasco-santos vasco-santos Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe @hugomrdias has an idea on the current state. More than having that merged, this will require changes across the stack as this will change how multiaddrs are created

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR should be made non-breaking just to get it finished, i tried a week ago or something but its troublesome. Need to look at it again.

rvagg and others added 3 commits March 16, 2021 18:09
Co-authored-by: Jacob Heun <jacobheun@gmail.com>
Co-authored-by: Vasco Santos <vasco.santos@ua.pt>
@momack2
Copy link
Contributor

momack2 commented Mar 16, 2021

libp2p/js-libp2p#830

@rvagg
Copy link
Contributor Author

rvagg commented Mar 22, 2021

In f1e6f2e I've tried to tighten up the scope of this project. It's important that this not become a catch-all for anything TS related and we actually have an end-point. I've also tried to fill out a lot of the missing parts of the project.

@anorth @jacobheun I'd really appreciate some reviews and feedback regarding how I've tried to define scope and some of the additional pieces. As noted there is risk of scope creep in this one and I'm keen to extract maximal value for this without it growing beyond the very rough estimates available at the moment.

@rvagg
Copy link
Contributor Author

rvagg commented Mar 22, 2021

latest (force pushed) commit is f0bc4c5 that has all the changes

@rvagg
Copy link
Contributor Author

rvagg commented Mar 22, 2021

urgh, f1e6f2e, sorry, mutable content addressing is hard, who knew?

@jacobheun
Copy link
Contributor

For scope, do we have a clear target that's driving the extra typing outside of completing the existing js-ipfs work? Right now, it's not clear to me what's driving the ipld-codec or multiformats work, although I have assumptions about why. Perhaps it would be useful to identify gaps in downstream projects like https://github.com/ceramicnetwork/js-ceramic? I briefly looked through the code and noticed there aren't a ton of custom types in the repo, but there are several places where ipfs calls are @ts-ignored'd. As one of the primary goals of this is for downstream type leverage, would selecting a couple of these larger projects help to validate/narrow our scope?

We have some existing TypeScript in the PL suite of JavaScript. This may expand as we have some developers who have a preference for working with strongly typed, or minimally type checked code. Annotations on our core libraries allows us to add tighter type checking to our CI process for any existing and new libraries that consume that code.

We already have [one example](https://github.com/ipfs/js-dag-service) of code being contributed to the PL stack from a third-party that uses TypeScript and could be improved by typing in the rest of our stack.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ease of onboarding new contributors is worth calling out specifically. When I joined, I found amount of code so overwhelming that I end up writing some type annotations so I could leverage tools to see what was passed around as opposed to trying to keep it all in my head.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth calling out that in the process of entyping js-ipfs and it's dependencies we have identified and fixed fare amount of otherwise unknown bugs in the code base.

@mikeal mikeal added confidence:high Confidence rating is 6 or above. ease:high Ease rating is 6 or above. impact:high Impact rating is 6 or above. labels Mar 25, 2021
@rvagg
Copy link
Contributor Author

rvagg commented Mar 29, 2021

Address comments in latest commit:

  • why the IPLD libraries are included (although I consider them less critical path here and they could be scrapped if we can't solve some of the issues as easily as I hope)
  • added some of @Gozala's comments re leverage/impact of this work
  • attempted to add success metrics by looking at downstream users, thanks to @jacobheun's suggestion

@rvagg rvagg mentioned this pull request Apr 10, 2021
77 tasks
@jacobheun jacobheun merged commit 4f299f8 into main Jun 9, 2021
@jacobheun jacobheun deleted the rvagg/typescript branch June 9, 2021 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confidence:high Confidence rating is 6 or above. ease:high Ease rating is 6 or above. impact:high Impact rating is 6 or above.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants