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(common): broaden types in StreamableFile constructor #9103

Merged
merged 3 commits into from
Feb 14, 2022
Merged

feat(common): broaden types in StreamableFile constructor #9103

merged 3 commits into from
Feb 14, 2022

Conversation

jonahsnider
Copy link
Contributor

@jonahsnider jonahsnider commented Feb 5, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

The StreamableFile constructor supports using a NodeJS.Buffer when passing a buffer (as opposed to a readable stream).

What is the new behavior?

The StreamableFile constructor accepts a Uint8Array when passing a buffer.
The parameter is checked using util's types.isUint8Array function, added in Node.js v10.0.0.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

NodeJS.Buffer extends Uint8Array so this will not affect existing code that uses NodeJS.Buffer instances.

Supporting broader types for parameters is generally good because it makes it easier for developers to take advantage of Nest's features.

Specifically, allowing Uint8Arrays in addition to NodeJS.Buffers makes it much easier to pass data buffers from WASM directly to StreamableFile without needing to pass them through Buffer.from beforehand.

@jmcdo29
Copy link
Member

jmcdo29 commented Feb 5, 2022

Code looks good to me. Thanks for updating the tests too

@jonahsnider
Copy link
Contributor Author

jonahsnider commented Feb 5, 2022

Unfortunately the tests are failing due to some very confusing behavior from chai:

test output

Trying to fix them currently...
Fixed.

@coveralls
Copy link

coveralls commented Feb 5, 2022

Pull Request Test Coverage Report for Build ab37a701-e40b-44ab-8605-d94a0aae68f6

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 94.131%

Totals Coverage Status
Change from base Build 6f937466-75ef-49c3-bc59-e84e673d5fb1: 0.001%
Covered Lines: 5694
Relevant Lines: 6049

💛 - Coveralls

@micalevisk
Copy link
Member

@jonahsnider could you leverage on this to run npm run format and commit packages/common/file-stream/streamable-options.interface.ts file as well? 😄

@kamilmysliwiec kamilmysliwiec merged commit 2b212eb into nestjs:master Feb 14, 2022
@jonahsnider jonahsnider deleted the feat/streamable-file-uint8array branch February 14, 2022 17:14
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.

5 participants