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

Create a clean stream using fs.createReadStream #764

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Its-Just-Nans
Copy link

@Its-Just-Nans Its-Just-Nans commented Nov 14, 2021

Relevant issues

#756
#634

The stream is piped in res. Next the stream generate an error The first argument must be one of type string or Buffer. Received type number and launchs the satus["500"]() function then the status["500"]() function modify headers (after the piped stream), that's the crashing error appear.

To counter that, the stream musn't create error, we can :

  • change stream = Readable.from(bytes) to stream = Readable.from(bytes.toString())
  • remove stream = Readable.from(bytes) because a correct stream is created with line 334

BUT using .toString() format the buffer and a different charset can produce an error (see toString()in npm run test).
This PR is the second (and working) option

Contributor checklist
  • Provide tests for the changes (unless documentation-only)
  • Provide explanation
  • Documented any new features, CLI switches, etc. (if applicable)
    • Server --help output
    • README.md
    • doc/http-server.1 (use the same format as other entries)
  • The pull request is being made against the master branch
Maintainer checklist
  • Assign a version triage tag
  • Approve tests if applicable

@Its-Just-Nans Its-Just-Nans changed the title Transfrom the buffer to string Create a clean stream using fs.createReadStream Nov 14, 2021
@zbynek
Copy link
Contributor

zbynek commented Aug 25, 2022

The idea was to reuse the stream and it works well with most supported versions of Node. Not sure how much of a performance issue it is to recreate the streamm and if it's worth fixing (#756 is not an issue with latest Node 12/14/16 and Node 10 is EOL)

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.

None yet

2 participants