search

multiformats / js-multiformats

Multiformats interface (multihash, multicodec, multibase and CID)
Other
232 stars 53 forks source link

When TypeScript? #249

Closed ukstv closed 10 months ago

ukstv commented 1 year ago

Considering lot of JS IPFS codebase is in TypeScript, when do you expect multiformats to be converted to TypeScript? What are the blockers?

rvagg commented 1 year ago

No, would this unblock something for you though? We should be exporting type definitions that are accurate as it's built into the build, testing and linting toolchain here so they're not just handcrafted. So I'd be interested to know what the advantage would be to users of us doing a TypeScript conversion here.

ukstv commented 1 year ago

So I'd be interested to know what the advantage would be to users of us doing a TypeScript conversion here.

When something weird happens in IPLD codebase, eventually you end up going through all the relevant code. Once in a while it leads to bunch of tabs open with TS code, and multiformats (and some others) in JS, which requires quite a context switch to grok.

wemeetagain commented 1 year ago

I'm in favor of converting this library to typescript. Several reasons:

achingbrain commented 1 year ago

My experiences of converting the js-libp2p stack to TypeScript have been a net positive, with some exceptions as to how we release changes to interfaces in external modules.

I don't think we could release changes with any real degree of confidence across so many modules without TypeScript. Obviously tests are king in all this but simple type checking has removed a whole class of bugs.

Doubling down on that using JSDoc to generate types places a lot of limits on what you can do, for example, interface support isn't there so you normally end up with some typescript in your project anyway.

JSDoc is also full of weird gotchas like when you use @typedef {import('./foo.').Bar} Baz if you check the generated .d.ts file you get a new type rather than reusing the existing one. This is fine for TypeScript as the new type has 100% overlap with the old one but it causes problems for documentation generators since they document both types.

The type contortions we regularly do with JavaScript aren't possible with TypeScript, at least, it's possible if you try really hard but generally it's much less effort to just do the simple thing, then you end up with a codebase that is much easier to follow and everyone wins.

Finally the type annotations proposal has reached stage 1 so this stuff might be coming anyway, we'd just get slightly ahead of the curve.

rvagg commented 1 year ago

ugh, well, I suspect @Gozala would be on board with this so that would leave me as the lone holdout in terms of who's left maintaining this stuff, so I suppose I won't be a blocker if someone wants to take on the task of converting this and making it 1:1 compatible with what we publish now.

p-shahi commented 1 year ago

@rvagg I believe @wemeetagain has already done the conversion here https://github.com/multiformats/js-multiformats/pull/251

Gozala commented 1 year ago

ugh, well, I suspect @Gozala would be on board with this so that would leave me as the lone holdout in terms of who's left maintaining this stuff, so I suppose I won't be a blocker if someone wants to take on the task of converting this and making it 1:1 compatible with what we publish now.

Here are my sentiments in no particular order tl;dr While I won't fight for this, I do find context switching to be worthwhile tradeoff for:

  1. Keeping run / eval without compilation flow
  2. Not affecting less TS inclined contributors (like @mikeal and @RangerMauve) productivity

Here is the longer version:

  1. 💚 TS syntax is more concise, easier to read and write
  2. 💔 Having a compilation step while fairly common is still annoying especially if you frequently use of the REPL, I often do and I think pure JS camp folks de also.
    • If we nodejs was more like deno this probably would not matter
  3. 💔 I find debugging compiled TS code in browsers to always be inferior to debugging the code that simply used JSDoc annotations.
    • I'm sure there is some way to resolve this, but in practice it usually is a problem
    • I find this context switch to be a lot more pain than switching between TS syntax and JSDoc syntax
  4. 💔 Error stacks traces are not accurate
    • Again can be resolved, but in my experience for one reason or another I do find myself having to figure where errors actually occured.
  5. 💚 TS syntax, unlike JSDoc is free of the gotchas that @achingbrain has outlined.
    • For what it's worth those can be addressed, more on that later

At the end of the day I personally care about benefits type checker brings and not about the new notation that TS team has invented. If I have to deal with compilation, I would much rather write in more proper typed language like Rust.

Dealing with all the TS in JSDoc quirks

Doubling down on that using JSDoc to generate types places a lot of limits on what you can do, for example, interface support isn't there so you normally end up with some typescript in your project anyway.

Yeah you can't define interface next to implementation. I do however think that forcing you to separate interface definition from implementation is a nudge into right direction. Our team settled on pattern where we define interfaces in a separate api.ts file providing interface definitions, which is then imported from the file defining implementation claiming compliance via @implements tag.

That is annoying indeed and TS team seems to have no interest fixing (probably they are more focused on type annotations syntax proposal).

We address above issue by not doing import typedefs, instead we just import api.ts file as a module, which unfortunately at runtime requires empty js files but not a big deal in practice.

I believe that also avoids doc generation issues.

The type contortions we regularly do with JavaScript aren't possible with TypeScript, at least, it's possible if you try really hard but generally it's much less effort to just do the simple thing, then you end up with a codebase that is much easier to follow and everyone wins.

Again jsdoc syntax is definitely not as pleasant to read / write but I'm not sure if I've encountered a case where I could not express something I needed and would be curious to know the examples. I think TS jsdoc syntax is not as well documented, but feature wise other than interface and abstract classes I don't think they're really apart.

Conclusion

I don't think switching to TS syntax is a clear win, at leas not until type annotations syntax extension has shipped in JS engines (and when that happens I highly suspect some TS syntax will also change or get deprecated).

I would suggest carefully weighting tradeoffs before going ahead with this. Either way I'm not going to stand in they way.

As an aside this might be a good opportunity to exercise community driven collective decision making

Gozala commented 1 year ago

I think we should also let @hugomrdias weave in on this, who used to advocate for TS in JSDoc, (and convinced me it was better tradeoff than TS which I used to be in favor of) yet more recently had to work with pure TS code base, unless I'm mistaken.

matthewkeil commented 1 year ago

I tend to agree with @wemeetagain and @achingbrain. TS is pretty much the industry standard such that its syntax is being incorporated (almost the same) into base JS with the type annotations proposal (that I am very much looking forward to!!). It will bring consistency to what most large projects already use. While it is a hurdle to initially get over, it is well worth it for both community inclusion, developer experience and modernization of syntax. +1 vote for TS.

nazarhussain commented 1 year ago

I personally believe Typescript vs JS+JSDoc debate should be over by now.

There were so many known issues in vanilla JS, because of which Tyepscript was eventually inevitable, and not all those issues can be addressed by adding up a few JSDoc comments on top of JS code. If that were the case community would have evolved more towards it, instead of adopting the TS ecosystem. If one wants to go with the generic evolution of the ecosystem, then one must move to TS to make projects future-proof. The benefits it would bring to contributors are also not hidden a simple Google search can list thousands.

That was all from the contributor's side of view, how about the consumer? What do consumers need? Ultimately three things 1) Stable library 2) Better Typesafety 3) Best Documentation. For the first two points, TS can be very helpful to achieve for later points TS can provide a better value addition with less maintainability for mismatch types.

dapplion commented 1 year ago

Please, our life is hard enough already as to voluntarily opt-out of Typescript. It's not great, but it's the best thing we have now by far

hugomrdias commented 1 year ago

im still very much on ts with jsdoc side, theres 0 reasons to add extra steps to an already complex js tooling situation. the ts team is committed to jsdoc and they keep improving it, typedoc works perfectly with jsdoc now.

i subscribe to everything @Gozala said here

for everyones reference here is a big project moving from ts to jsdoc ts https://devclass.com/2023/05/11/typescript-is-not-worth-it-for-developing-libraries-says-svelte-author-as-team-switches-to-javascript-and-jsdoc/

If theres any type issues with using multiformats as it is now im available to help fix it but if theres no problems that need fixing why change to a worst solution ?

BigLep commented 1 year ago

So there's been a lot of good discussion here - thanks all for contributing.

My wants:

  1. get this unblocked one way or the other. (There are also some comments in https://github.com/multiformats/js-multiformats/pull/251, but I think we should discuss the "covert to typescript" discussion here rather than the PR.)
  2. make sure that we're solving some real problems here and avoiding unnecessary churn. I'm not a JS developer, but it seems like the JS ecosystem has these waves of changes and updates and I want to ensure we're not just getting caught up in the latest new thing. (I'm not saying that's happening and I recognize maintainers have been deliberate about TS conversion in other places like js-libp2p, js-ipfs, Helia, etc.). My hope here is that we're clear about the value prop.

Who ultimately are the maintainers of this repo? I don't think we've been explicit about this. Looking at commits the last year (https://github.com/multiformats/js-multiformats/graphs/contributors?from=2022-06-07&to=2023-06-07&type=c ), we've had:

I know @achingbrain is supportive of this work (as he approved the PR) and my understanding is that @rvagg and @Gozala aren't strictly against it but want to make sure this is really going to making things better in some way (e.g., help consumers, increase contributions, reduce maintenance for maintainers, increase the number of maintainer hours with more humans involved).

In this thread we have had some signal from consumers that this would be beneficial:

Ways I can see to unblock the decision-making here include:

  1. If others raised their hand to help with more of the maintenance as a result of this work. Basically, if as a result of the typescript conversion, would you be willing to do PR review and potentially tackle incoming issues?
    • (The process for triaging issues in this repo currently just happens during "IPLD triage", which is just @rvagg and I at the moment. Others are welcome to this, but even if we knew we could assign code reviews or small issues to others and they'd get done, that would be a win.)
  2. Synchronously gather and decide. One forum where some of the vested parties show up is the js-libp2p triage on Tuesdays, but we could also find another time. Let me know if you'd like this.
p-shahi commented 1 year ago

Per 2023-06-06 js-libp2p maintainers calls, @wemeetagain will add a diff of generated outputed types as well as do a drop in replacement with #251 into the node_modules folder and verify everything works (in addition to the already passing tests).

If others raised their hand to help with more of the maintenance as a result of this work. Basically, if as a result of the typescript conversion, would you be willing to do PR review and potentially tackle incoming issues?...even if we knew we could assign code reviews or small issues to others and they'd get done, that would be a win

We can have this convo in the upcoming 2023-06-14 js-libp2p maintainers call. Welcome others to join as well.

p-shahi commented 1 year ago

Update: @achingbrain and @wemeetagain are ok with taking on maintenance of this repo. It's mainly used in the libp2p (and Helia!) domain so it makes sense for the js-libp2p team to steward this.

So the answer to this

If others raised their hand to help with more of the maintenance as a result of this work. Basically, if as a result of the typescript conversion, would you be willing to do PR review and potentially tackle incoming issues?

is yes.

Should we add this repo to the js-libp2p triage/maintainers call then @rvagg @BigLep ? How do you propose we handle responsbilities there.

Having said that all that, would you be ok with approving the changes proposed in https://github.com/multiformats/js-multiformats/pull/251 @rvagg

rvagg commented 1 year ago

It's mainly used in the libp2p (and Helia!) domain

This isn't quite correct, the point of js-multiformats is that it sits at the core of all the PL stack for JavaScript users. It gets used by everyone regardless of whether they touch libp2p (e.g. in the 4.5 years I've been with PL, I've written very little JS code that touches libp2p directly, and not a whole lot more that touches it indirectly!).

Even within PL there's the web3.storage team that builds on this library in an entirely different direction to Helia and libp2p. Then there's all the folks outside of PL doing weird and whacky things!

But my point here is that yes, I would very much love to have maintenance help in this library but the thing I would really like to protect in all of this is the whole reason js-multiformats exists—to avoid bloat in everyone's dependency chains. Because this library sits within everyone's libraries and applications when they touch anything IPFS/IPLD/libp2p related (even remotely related!), it has to be lean, both on disk and within the import trees (for bundle sizes). Even adding seemingly trivial things like sha1 are a concern for this reason. That's why this library exists and why it looks like it does.

So, I'm very hesitant to just hand this off to the js-libp2p team in whole, but I'd very much like help in continuing to maintain it because I'm really not the best person to deal with the constant onslaught of people asking why it doesn't work for their very specific bundling or building toolchain (because that's the majority of issues we get for our simpler libraries!). That's also the lens I use for viewing this TypeScript question—does it help with any of that? I'm dubious, but open to being convinced because it seems a lot of the issues get filed by people complaining about typedefs (but it's not clear to me that continuing to fix what we have is not better than converting the entire thing).

hugomrdias commented 1 year ago

Exactly 💯, multiformats is literally every where and is used by bluesky, walletconnect, fission, ceramic and many many others (everyone using uint8arrays by @achingbrain 😁). Scroll through https://www.npmjs.com/browse/depended/multiformats to see direct dependents.

I available to help @rvagg maintain this repo.

wemeetagain commented 1 year ago

Looking through the dependent packages, the majority, weighted by downloads, ignoring deprecated, are actually typescript projects, by a lot. I didn't go thru the whole list, but I'd bet that the majority of all dependent packages, the long tail of weird and wacky things, are typescript projects too. It's probably worth considering that most of our users want first-class support for it and that it's not exactly a niche toolchain.

I feel that the current direction, jsdoc-generated typedefs, has lots of pain points for the sake of simplicity. But from the perspective of a typescript consumer or maintainer, its actually more complex and error prone.

What I mean is that for a typescript consumer and maintainer, a build step is still required (npm run build). Its just that there's poorer cohesion, less confidence, more bugs (and open issues) between the logic and the generated types. All the 'complexity' for a poorer process with much less gain. More sad consumers and more maintainer(s) time wasted.

Leanness is a real concern, but typescript shouldn't hurt. There are several approaches as far as which files to publish, but ultimately import tree size won't change, and disk leanness doesn't have to change. (Although it may be desirable for typescript consumers to also have the typescript code bundled, thats more of a publishing choice).

I feel like a slimy typescript salesman, but I'll end with this. Typescript has moved beyond 'js framework of the month' and into a more noble role because it actually moves the needle on coordination, productivity, and code correctness. Codified interfaces scale javascript development beyond small teams. And integrating these interfaces and types into the code provide opportunities for maintainers to better and more confidently write, organize, and refactor their codebases. In the open-source world, this proves quite valuable. Consumers rely on a host of software packages they didn't write. Consumers are often producers or maintainers. Everyone is using someone else's code, and navigating and extending the huge web of code. In the typescript ecosystem, package functionality has structured inputs and outputs and typechecking that prevent whole classes of bugs. This structure provides insight and clarifies ambiguities in a new codebase, which is invaluable to a consumer or or new contributor. The interfaces and types provide the slick veneer over javascript that's needed to succinctly describe code shape and hint at behavior. The type checking allows developers to drop type constraints from their mental buffer. All of these factors ultimately translate to less friction and higher productivity.

Gozala commented 1 year ago

I would really encourage to focus conversation around the problems been solved / introduced, inconveniences been solved / introduced and restraining from other sentiments and speculations like ES syntax extensions that JS engines will implement and ship. I have no doubt you are all well meaning, but I can't help but suspect bias. When it is true and JS engines adopt TS syntax, arguments voiced against TS syntax adoption will no longer be valid making transition a win, which I would whole heartedly support. Until such time, plans that JS engines may have do not address any arguments voiced against it. So please lets focus on facts today when facts change we can revisit decisions that were based on them.

So far I'm hearing following arguments (please correct me if I have left something out):

While #251 mentions some issues, it does not provide specific details so I'm not sure how to capture it.

  1. Inconvenience of context switching when TS projects use this.
  2. Certain limitations of jsdoc syntax (which per my comments can be worked around)
  3. Certain inconvenience of TS when using repl / browser console (can't copy & paste code etc...)
  4. Certain inconvenience when using debuggers due to source map issues

If we have a complete list of problems / inconveniences we could use empirical method, comparing tradeoffs to come to a decision.

I also should mention that contribution list only accounts for commits, which fails to account for reviews, that I have personally been involved in and have intention to continue to do so (regardless of decision here). Worth mentioning that rest of the contributors (excluding @juliangruber) include @RangerMauve and other web3.storage members, who I suspect fall into the jsdoc side of the argument.

BigLep commented 1 year ago

Lots of conversation here and I do want to get a decision made.

@wemeetagain : are there other arguments you think should be added to @Gozala 's list?

@wemeetagain : not critical here, but can you link to how you generated the consumer results above?

@Gozala :

contribution list only accounts for commits, which fails to account for reviews Ack and understood. My quick look at above commit contributors was to get a pulse on this. I think my original question stands that needs to be answered (and documented): "Who ultimately are the maintainers of this repo?"

Below is a more exhaustive list of contributors for different actions over the last 12 months:

actor Issue Open Issue Comment Issue Close PR Open PR Comment PR Review PR Close Release Publish Grand Total
rvagg   68 10 1 28 46 21 1 175
Gozala 10 38 1 5 23 23 2   102
achingbrain 3 37 4 12 8 11 5   80
RangerMauve   23   1 3 10     37
BigLep   9 2           11
ukstv 1 4             5
pablomendezroyo 1 4             5
wemeetagain   3   1         4
p-shahi   4             4
hugomrdias   1     1 2     4
vmx   3             3
pleerock   1   1     1   3
olizilla 1 1   1         3
vikiival 1 1             2
tabcat 1 1             2
shamilovtim   2             2
misterupkeep 1 1             2
i-norden       1     1   2
delaneyj 1 1             2
cooleydw494 1 1             2
alanshaw       1   1     2
panick-carousel 1               1
nazarhussain   1             1
mistermoe       1         1
mikeal           1     1
matthewkeil   1             1
MarcoPolo           1     1
lidel   1             1
Latrasis 1               1
juliangruber       1         1
honungsburk 1               1
garyrob 1               1
galargh             1   1
dapplion   1             1
Grand Total 25 207 17 26 63 95 31 1 465

Data Source

p-shahi commented 1 year ago

A quick update: discussions on the next steps will stall for a week or two as js-libp2p maintainers and IPLD maintainers are out of office

sarvalabs-sai commented 1 year ago

Hey @p-shahi I'm not able to import this package in nodeJS project written in typescript. throws No "exports" main defined in ./node_modules/multiformats/package.json error.

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
}
SgtPooki commented 1 year ago

Here is the longer version:

  1. 💚 TS syntax is more concise, easier to read and write

100%, it's less maintenance, and there is much less chance for your interface types to mismatch your source code.

  1. 💔 Having a compilation step while fairly common is still annoying especially if you frequently use of the REPL, I often do and I think pure JS camp folks de also.

    • If we nodejs was more like deno this probably would not matter

ts-node makes this a non-issue. You can REPL ts-code just as well, and many people in the TS ecosystem do, if they don't just use deno or bun.

  1. 💔 I find debugging compiled TS code in browsers to always be inferior to debugging the code that simply used JSDoc annotations.

    • I'm sure there is some way to resolve this, but in practice it usually is a problem

This is likely due to build configuration or minification problems. source maps will give you pure JS to look at in the browser, and I've rarely had issues here. One problem I've run into that can be bypassed with some minor setup is workspace files in chromium editors, where you can change the code directly in your devtools. However, this is likely only relevant for front-end devs and mitigated easily with live-reload.

  • I find this context switch to be a lot more pain than switching between TS syntax and JSDoc syntax

I completely disagree here. Having migrated a number of projects to Typescript, I have to call out that the time you spend in that pain will lessen significantly, whereas the disconnect between JSDoc types and your actual code (regardless of ts-check pragma) will cause much more pain.

  1. 💔 Error stacks traces are not accurate

    • Again can be resolved, but in my experience for one reason or another I do find myself having to figure where errors actually occured.

Are we talking stacktraces from a binary in a terminal, browser stacktraces, or reported stacktraces in an analytics/metrics dashboard/tool?

I have run into this as well, but depending on where you're viewing the stacktraces and what source-maps you have, this is a one-time fix, or at most a minor inconvenience because more of your errors are caught during compile-time with typescript.

  1. 💚 TS syntax, unlike JSDoc is free of the gotchas that @achingbrain has outlined.

    • For what it's worth those can be addressed, more on that later

Yeah you can't define interface next to implementation. I do however think that forcing you to separate interface definition from implementation is a nudge into right direction.

Sure, but the solution you provide is you writing typescript, and you can have the original source be typescript and still write your interfaces in a separate TS file.

We address above issue by not doing import typedefs, instead we just import api.ts file as a module, which unfortunately at runtime requires empty js files but not a big deal in practice.

You write typescript, to solve the problems JSDoc has.

but I'm not sure if I've encountered a case where I could not express something I needed and would be curious to know the examples.

When you start using more generics and higher-kinded types, you will run into many problems, and end up writing typescript.

No matter how painful the disconnect between JSDoc and source can get (or any other concerns here), my main issue with JSDoc types is Generics. And with this library, better generics are an absolute must.

This isn't quite correct, the point of js-multiformats is that it sits at the core of all the PL stack for JavaScript users. It gets used by everyone regardless of whether they touch libp2p (e.g. in the 4.5 years I've been with PL, I've written very little JS code that touches libp2p directly, and not a whole lot more that touches it indirectly!).

We need generics; more & better generics because of exactly where this library sits in the ecosystem. And once we have that, we can improve typings in consuming libraries by enhancing our types with better conditional types.

I think there have been a lot of discussion around how JSDoc can work and currently is, but I haven't seen any challenges to whether typescript is better for ensuring better type-safety in the source code. So it really seems like the main problems are:

  1. actually doing the TS update (mostly done, but needs validation on type diff #251 )
  2. maintaining TS code going forward.

and for 2, many of the workarounds/solutions @Gozala is already using involve directly writing typescript anyway.

Gozala commented 1 year ago

Are we talking stacktraces from a binary in a terminal, browser stacktraces, or reported stacktraces in an analytics/metrics dashboard/tool?

The problem is that it's all of the above and more. Again as you pointed out you can address most if you put enough effort into them, however my point is precisely that it requires putting some effort every time which is a tradeoff.

and for 2, many of the workarounds/solutions @Gozala is already using involve directly writing typescript anyway.

I am not against typescript, I'm just saying you can apply it where it actually provides value as opposed to just rewriting everything in TS because all the other code is TS. I tend to apply TS syntax for code that does not have runtime representation to take advantage of type checking.

We need generics; more & better generics because of exactly where this library sits in the ecosystem. And once we have that, we can improve typings in consuming libraries by enhancing our types with better conditional types.

There is a implied assumption here. Perhaps it would be a more productive direction to explore what better generics would look like and what problems would they be solving. Don't get me wrong I think they can be extremely useful along with phantom types, yet in my experience overusing them can introduce unnecessary complexity (I suppose dI'm siding with Elm camp here, but that is irrelevant).

Naturally if there are some improvements that we can not make with current setup will be a good case for changing it.

wemeetagain commented 1 year ago

There are improvements that we cannot make with the current setup.

It was only after rewriting this library in typescript (#251) that some refactoring opportunities became visible to me.

I think we would like to further simplify this library, if possible, but due to vanilla javascript coding paradigms, verbosity of jsdocs, lack of confidence of correctness of large code changes, larger refactoring of the codebase is sadly out of the question in its current state.

Typescript provides value by simplifying maintenance burden.

SgtPooki commented 1 year ago

@wemeetagain Action items discussed yesterday

BigLep commented 1 year ago

Following up on the comment above...

js-libp2p and Helia maintainers met during a colo. We understand this isn't a clear-cut answer and that this increases some friction for some contributors/users.  There is a weight of opinion across the Helia and js-libp2p teams who contribute and use this library to move this TS migration forward. We're planning to make this happen by 2023-10-03 unless another maintainer says "no" sooner.

Before merging https://github.com/multiformats/js-multiformats/pull/251, the plan is to:

  1. Update the PR to include any missed commits
  2. Bubble up the change (unreleased) to js-libp2p to ensure all tests still pass there. (The reason being that js-libp2p hits a lot of js-multiformats' API surface area and can be an extra validation point that didn't screw anything up.)
  3. Prepare the release as a new major version.

That said, this issue has also highlighted ownership ambiguities. I have created a separate issue to handle this (https://github.com/multiformats/js-multiformats/issues/273 ).

ukstv commented 1 year ago

This is great @BigLep !! Finally a decision, that removes friction!

ukstv commented 10 months ago

Yahoo!