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

feat: add types #9

Merged
merged 19 commits into from
Sep 2, 2020
Merged

feat: add types #9

merged 19 commits into from
Sep 2, 2020

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Jul 30, 2020

This pull request does following:

  • Adds JSDoc type annotations across the board
  • Adds npm run check command that runs a type checker.
  • Adds type checking phase to the CI
  • Adds the npm run build and npm run build:types scripts that generate .d.ts files in dist.
  • Updates naming / wording to reflect the fact that functions operate on iterables instead of iterators.
  • Adds typesVersions field to package.json so that programs that install these libraries will get types out of the box.
  • Fixes the issue that TS uncovered in it-multipart (error was swallowed)
  • Adds .npmignore which doesn't ignore package/*/dist directories so they are published with a package.

This also cherry-picks #11 to overcome playwright-test issues

@@ -19,7 +38,7 @@ async function * multipart (stream, boundary) {
const headerEnd = Buffer.from('\r\n\r\n')

// allow pushing data back into stream
stream = prefixStream(stream)
const stream = prefixStream(source)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 Type checker would not allow overloading same var here because it's type is IncomingMessage which is different from PrefixPush.

@Gozala Gozala force-pushed the types branch 2 times, most recently from 67521f0 to c0da7bd Compare July 30, 2020 09:07
for await (const tasks of batch(source, size)) {
/** @type {Promise<{ok:true, value:T}|{ok:false, err:Error}>[]} */
const things = tasks.map(p => {
return p().then(value => ({ ok: true, value }), err => ({ ok: false, err }))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 Type checker can refine union types as long as they share a common property unique per variant. That is why ok field was added. That way type checker can infer type of result in both branches of if (result.ok).

@@ -143,6 +198,7 @@ function waitForStreamToBeConsumed (stream) {
return next
} catch (err) {
pending.reject(err)
throw err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

📝 TS rightly points out that without throw here iterator did not comply with Iterator<T> interface because next() returns { done:boolean, value: T }|undefined (undefined is returned because catch was swallowing an error). Added throw here so it type checks & I think that is also what code supposed to do.

return new globalThis.ReadableStream({
/** @type {UnderlyingSource<T> & { _cancelled:boolean} } */
const pump = {
_cancelled: false,
async start () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From what I understand given coveralls report it seems to conclude that coverage has decreased because start here is never called. Seems like a wrong conclusion, because all this change does is moved ReadableStream parameter to pump variable so it's easier to type annotate.

I can awkward inline type annotation, or suppress TS error (which otherwise complains about unknown _cancelled field). But it seems like ignoring coverall here might be a best option.

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 changed it to do inline thing to workaround coveralls.

@Gozala
Copy link
Contributor Author

Gozala commented Aug 4, 2020

@achingbrain just putting this on your radar, in case you missed it

Copy link
Owner

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

This looks good, a few minor nits but nothing major.

Thanks for submitting it!

packages/it-flat-batch/index.js Outdated Show resolved Hide resolved
packages/it-flat-batch/index.js Outdated Show resolved Hide resolved
packages/it-drain/README.md Outdated Show resolved Hide resolved
packages/it-first/index.js Outdated Show resolved Hide resolved
packages/it-map/index.js Outdated Show resolved Hide resolved
packages/it-multipart/index.js Outdated Show resolved Hide resolved
Gozala and others added 7 commits August 26, 2020 07:03
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
Co-authored-by: Alex Potsides <alex@achingbrain.net>
@Gozala Gozala requested a review from achingbrain August 26, 2020 14:24
@achingbrain achingbrain merged commit 57775a6 into achingbrain:master Sep 2, 2020
Copy link

🎉 This PR is included in version 1.0.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants