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

Bump readable-stream version #618

Merged
merged 1 commit into from
Nov 20, 2023
Merged

Conversation

frankhinek
Copy link
Contributor

@frankhinek frankhinek commented Nov 19, 2023

Summary

This PR bumps the versions of the readable-stream and @types/readable-stream dependencies.

Context

While troubleshooting an issue with RecordsWrite overwrites in web5-js I attempted to use a locally linked dwn-sdk-js package to add logging but encountered a series of errors that prevented building from completing successfully:

src/dwn-api.ts:434:43 - error TS2345: Argument of type '{ descriptor: Descriptor & RecordsWriteDescriptor; authorization: AuthorizationModel; recordId: string; contextId?: string; ... 4 more ...; target: string; }' is not assignable to parameter of type 'RecordOptions'.
  Type '{ descriptor: Descriptor & RecordsWriteDescriptor; authorization: AuthorizationModel; recordId: string; contextId?: string; ... 4 more ...; target: string; }' is not assignable to type '{ author: string; target: string; encodedData?: string | Blob; data?: ReadableStream<any> | Readable; }'.
    Types of property 'data' are incompatible.
      Type 'Readable' is not assignable to type 'ReadableStream<any> | Readable'.
        Type 'import("/Users/frankhinek/Code/dwn-sdk-js/node_modules/@types/readable-stream/index").Readable' is not assignable to type '_Readable.Readable'.

434           record = new Record(this.agent, recordOptions);
                                              ~~~~~~~~~~~~~

src/record.ts:191:7 - error TS2322: Type 'Promise<Readable>' is not assignable to type 'Readable | Promise<Readable>'.
  Type 'Promise<import("/Users/frankhinek/Code/dwn-sdk-js/node_modules/@types/readable-stream/index").Readable>' is not assignable to type 'Promise<_Readable.Readable>'.
    Property 'compose' is missing in type 'import("/Users/frankhinek/Code/dwn-sdk-js/node_modules/@types/readable-stream/index").Readable' but required in type '_Readable.Readable'.

191       this._readableStream = this._agent.processDwnRequest({
          ~~~~~~~~~~~~~~~~~~~~

  ../../node_modules/@types/readable-stream/index.d.ts:302:9
    302         compose<T extends NodeJS.ReadableStream>(stream: T | ComposeFnParam | Iterable<T> | AsyncIterable<T>, options?: { signal: AbortSignal }): T;
                ~~~~~~~
    'compose' is declared here.

The root cause is that dwn-sdk-js and web5-js are using significantly different versions of the @types/readable-stream type definitions:

  • dwn-sdk-js depends on 2.3.15 which was published over a year ago (31-Oct-2022)
  • web5-js depends on 4.0.0 which was published ~4 months ago (25-Jul-2023)

Since dwn-sdk-js has been using v4.4.0 of readable-stream for 5+ months it seems logical that it should also use more recent type definitions.

Changes

  • Bumps the @types/readable-stream version from 2.3.15 to 4.0.6.
  • Bumps the readable-stream version from 4.4.0 to 4.4.2.

Notes

  • web5-js has been using v4.4.2 of readable-stream for 3 months without any known issues. The same commit also bumped the type definitions from 2.3.15 to 4.0.0.
  • Here are the release notes for readable-stream v4.4.1 and v4.4.2 for reviewers to confirm they are comfortable with the changes.
  • A related PR was opened for web5-js to bump the @types/readable-stream version from 4.0.0 to 4.0.6.

Copy link

codesandbox bot commented Nov 19, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6c01ea2) 98.56% compared to head (2c69bc7) 98.56%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #618   +/-   ##
=======================================
  Coverage   98.56%   98.56%           
=======================================
  Files          68       68           
  Lines        8377     8377           
  Branches     1219     1219           
=======================================
  Hits         8257     8257           
  Misses        114      114           
  Partials        6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mistermoe
Copy link
Contributor

evidence in favor of us maintaining package(s) that include common utilities that all of our other libs/sdks depend on. e.g. @web5/common could include readable-stream utilities and types. @web5/common can then be used in all other packages e.g. @web5/dids, @web5/credentials, dwn-sdk-js etc.

Same would apply for cryptographic utilities. prevents us from running into drift issues like these. just need to update 1 lib and the effects cascade through.

@frankhinek frankhinek merged commit bb881cb into main Nov 20, 2023
4 checks passed
@frankhinek frankhinek deleted the frankhinek/bump-readable-stream branch November 20, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants