Added integrated Typescript typings

[alexandercerutti]

Hello there, I'm working on a migration to Typescript of a library that uses a very old version of bl (v1.2.3 - which, for unknown reasons, is unlisted on NPM to me, but still accessible via URL).

I'm not a very huge fan of DefinitelyTyped but I DefinitelyLove Typescript. Also, I was looking at @types/bl, and they seemed a bit wrong to me, based on the implementation.

So I decided to rewrite them with a better structure to represent how the code was written (in fact interfaces allow to specify of both a constructor and a function to be executed) and inside the package.

Now all the methods include a documentation comment based on the one in the Readme.

I also tried to run the tests available inside @types/bl, and they seem to be okay, and run linting.

I hope this is appreciated. Let me know if there's anything that you feel is wrong, and I'll feeex it! 🛠️

(EDIT: Moved to draft, I found a few issues when trying to integrate the types inside the project I'm migrating to TS - still attempting to fix. Meanwhile, feel free to comment).

P.s. I'll handle deprecation of @types/bl on DefinitelyTyped if this PR will get accepted.

[mcollina]

wdyt @rvagg? Should we ship types?

[rvagg]

Yeah, I don't mind shipping types, but checking correctness of manually generated types and keeping them in sync is the challenge. I've mostly been going with the approach of annotations and using tsc to compile type definitions from my JS and ship that. But doing that means making sure the annotations are complete and correct through the whole codebase, which can be a bit tedious.

We could ship this and cross fingers, and hope that people show up in the future to adjust and correct the over time, but it'll always be a catch-up effort as TypeScript evolves and our dependencies evolve too.

[mcollina]

@alexandercerutti could you add some https://www.npmjs.com/package/tsd tests?

[alexandercerutti]

and hope that people show up in the future to adjust and correct the over time, but it'll always be a catch-up effort as TypeScript evolves and our dependencies evolve too.

This is what happens today, just they are not under your domain but under DefinitelyTyped's, isn't it?

could you add some tsd tests?

@mcollina I can try adding them, sure, but are we sure this is needed? I mean, as per tsd statement (

These .test-d.ts files will not be executed, and not even compiled in the standard way

) a typings test allow to check the types... but as the per main concern (if I got it correctly) if the JS API changes and the test do not get updated, tests will still pass, don't you agree? This is also basically what happens on DT.

Maybe the best way for testing the whole thing, would be using something like ts-node with allowJs config set, but I'm not sure this won't be a bit overkill...

EDIT: I also tried in other projects tests written in JS but type-checked with //@ts-check on the file beginning.
We might think to a test that uses current tests with the help of JSDoc. They will get for sure edited when API changes...

[alexandercerutti]

@mcollina @rvagg I've chosen a different approach, which I think is more reliable in terms of testing types: type-checking existing tests.

Luckily for us, Typescript supports a subset of JSDoc, so I sightly changed test/test.js to support type-checking, added typescript as a dev dependency, and a new script in package.json, to check the file without emitting any compiled file but just errors, in case.

By doing so, I also discovered some cases that were not supported by the declarations I created.

For example, methods like BufferList.readIntLE were not supporting the second parameter (and they still don't, based on @types/bl), but they should, so I changed their signature to inherit it from NodeJS.Buffer.

I also added types support for BufferList and BufferListStream to Uint8Array and number.

Just a few notes and questions:

• Since tests use a "private" variable BufferListStream._bufs to check the correctness, I had to create a @typedef to add it to BufferListStream without exposing it inside typescript typings. This might become a little critical point in case of changes IMHO. I don't really think that is very correct to use a private variable in tests, but I cannot come out with a different solution that is not a getter.

• Just for a matter of coherence, I renamed BufferList in BufferListStream import in test/test.js, cause the default one is BufferListStream. It was confusing.

• I noticed that, for what concerns duplicate test, bl.prototype and dup.prototype is checked.

bl/test/test.js

Lines 794 to 797 in ccff4dc

	const bl = new BufferListStream('abcdefghij\xff\x00')
	const dup = bl.duplicate()
	t.equal(bl.prototype, dup.prototype)

I've added two logs to see more. Sadly tape seems to suppress console.log.

But this test is always to be true because both seem to be undefined (uh?). In fact, BufferStreamList merges the prototype of BufferList, but it not extends (was here the concern that Object.setPrototypeOf() is "slow" ?).

bl/bl.js

Line 37 in f7a0071

Object.assign(BufferListStream.prototype, BufferList.prototype)

I might be missing something here. But I'm not sure if this is correct...
Anyway, to add a type for the prototype, I set a generic "Object" for both BufferList and BufferListStream.

• I noticed that a package-lock.json is missing here. Should it get added (maybe in a different PR)?

• I noticed that this repo misses a configuration setting (like prettier or .editorconfig). I had to challenge my editor to attempt to keep styles as coherent as possible (it was changing everything in tests and typings). Should a configuration get added (maybe in a different PR)?

---

To test, take index.d.ts and remove | BufferListAcceptedTypes; from BufferListStreamInit; then run tests.

https://github.com/alexandercerutti/bl/blob/9b0ae38a92cabd4250d9434e80d2103abf017464/index.d.ts#L8-L10

Let me know what you think about this, any concerns, or if I should revert this at all and proceed with tsd tests.

[rvagg]

some brief drive-by thoughts while I have some time to glance at this:

• no package-lock.json please
• no .editorconfig please
• I think the Object.assign() looks right? it's copying properties from one prototype to the other, not making an inheritance or equivalence
• the duplicate test does look wrong, I think the assertion should probably be t.same(Object.getPrototypeOf(bl), Object.getPrototypeOf(dup))
• I think you should just be able to // @ts-ignore the _bufs usage in the test to make it not be concerned about that, I do this pretty regularly when I'm running TS across JS tests that are doing funky things that you normally wouldn't do in TypeScript - it's pretty common to be testing edges of these kinds of things in JS that you wouldn't do in TS because you have strong asssumptions (like .. in my JS I tend to build in type checking typeof foo === 'string' kind of thing, but TS folks don't tend to do that because foo:string, but when you use that from JS you lose all guarantees, you really should be checking, so in tests you should be throwing numbers and objects to make sure it handles wrong types properly).
• faucet is getting in the way of test output for you, see the "test" script that pipes through it. Just run node test/test.js to get raw TAP output, and tape.only() the test to run just that one.

Otherwise I think this seems reasonable? I still wouldn't mind doing a compilation of the types from annotated source, I do this on most of my new projects these days and have been slowly retrofitting older ones I use a lot. https://github.com/rvagg/iamap is an example of this - npm run build:types will regenerate the types/ directory. There's a basic interface.ts in there that has some things that are best described outside of annotations but otherwise the definitions are all pulled from the JS source annotations.

I'm not saying this is necessary here, it could be a stretch goal, but we may find the output differs when we do it like that.

[alexandercerutti]

@rvagg thanks for the reply

• Just to ask you: why no package-lock.json and something like a .editorconfig?
• Okay yeah. I actually never thought that copying could be a solution actually. I always used Object.create in case.
• After a test, both bl and dup result in Duplex objects with methods. Yep, that feels correct. I'll open a new PR after this.
• I actually don't really like using @ts-ignore if I can avoid it. I mean, I've used it for an issue with BufferEncoding in node types because that's out of my control, but I'd avoid using it because it might cause problems in the future (if errors get suppressed, types checks might go error and we could not know that - also that would mean to suppress the whole line, so there could happen other errors).

in my JS I tend to build in type checking typeof foo === 'string' kind of thing, but TS folks don't tend to do that because foo:string

This is a very bad behavior from ts folks - assuming that Typescript can completely replace JS - I agree with you.
That said, I think that typing just through JSDoc is a good compromise that does not expose _bufs and that allows us to be sure everything works as expected.

• I usually like to generate declarations through tsc too. I tried to make them be generated, but as I expected TS cannot infer correctly that BufferListStream "extend" Duplex and BufferList. I've looked a bit at your iamap but it seems to not have such cases, so your technique is valid as well.

That would be possible in this library, maybe, by using classes (but that would mean to sacrifice BufferList() and BufferListStream() I guess) and by not using inherits anymore (which is considered legacy now).

[mcollina]

lgtm

[alexandercerutti]

Hey @rvagg, just to remember you about this :)

[mcollina]

@rvagg I can release if you are ok with the approach.

[alexandercerutti]

Hey @rvagg, do you have any updates about this?

[piranna]

Any update on this? What's missing to be merged?

[rvagg]

sorry, seems fine to me I think, just some minor things in package.json and we can get this out

[alexandercerutti]

@rvagg done!
What about if, after this PR, we do set up a prettier or a .editorconfig? I think I mixed up a little bit the syntax due to my default config in vscode. 🤔

[rvagg]

if you can come up with an .editorconfig that isn't too disruptive then I think that'd be fine

[alexandercerutti]

I'll try to keep styles as similar as possible with the original code style 👍
I'll start a new branch starting from this one, so I'll format everything

[piranna]

Should I start a branch too for the Int64 support of #109, or should I wait to get this merged first?

[github-actions]

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[alexandercerutti]

Great! I'll now proceed with requesting DefinitelyTyped types deprecation!