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

fix: switch to undici for requests to fix stream close errors #666

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

austinkelleher
Copy link
Contributor

Fixes #626

When updating WPT resources, the wpt command would sometimes throw a ERR_STREAM_PREMATURE_CLOSE error. Switching to undici fixes this issue.

The undici fetch function is only supported on Node 16+. After discussing with the team, we agreed that dropping support for Node 14 in this tool was acceptable (#626 (comment)).

➜  node git:(main) ✗ ./node /Users/austinkelleher/Documents/open-source/node-core-utils/bin/git-node.js wpt resources
---------------------- Checking updates for resources... -----------------------
Last local update for resources is fbf1e7d24776
✔  Last update in upstream is 66933c819b6e
✔  Read asset list, 165 files in total.
Excluded 133 files, 32 files to write.
--------------- Writing assets to test/fixtures/wpt/resources... ---------------
✔  Downloaded 32 assets.
✔  Updated test/fixtures/wpt/versions.json
✔  Updated test/fixtures/wpt/README.md
✔  Updated test/fixtures/wpt/LICENSE.md.

When updating WPT resources, the `wpt` command would sometimes
throw a `ERR_STREAM_PREMATURE_CLOSE` error. Switching to `undici`
fixes this issue.

The `undici` `fetch` function is only supported on Node 16+.
After discussing with the team, we agreed that dropping support
for Node 14 in this tool was acceptable.
@codecov
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

Base: 83.41% // Head: 83.41% // No change to project coverage 👍

Coverage data is based on head (5b81893) compared to base (51a3b24).
Patch has no changes to coverable lines.

❗ Current head 5b81893 differs from pull request most recent head a9c0974. Consider uploading reports for the commit a9c0974 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #666   +/-   ##
=======================================
  Coverage   83.41%   83.41%           
=======================================
  Files          37       37           
  Lines        4131     4131           
=======================================
  Hits         3446     3446           
  Misses        685      685           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Feb 19, 2023

The commit message would need to be updated to reflect that this is a breaking change. Calling it fix is probably not right as we're dropping support for Node.js 14.x. We may want to add an "engines": { "node": ">=16.8.0" }1 entry in the package.json as well.

Footnotes

  1. according to https://github.com/nodejs/undici#undicifetchinput-init-promise, that's the minimum Node.js version which is compatible.

@targos
Copy link
Member

targos commented Feb 20, 2023

For whoever is going to merge: add a line with BREAKING CHANGE: description of the change to the commit message to make the version bump happen.

@austinkelleher
Copy link
Contributor Author

@aduh95 @targos Thanks for all of the great feedback. Let me know if you want me to make updates to the commit history.

Can someone please trigger the CI for me? Thanks!

@aduh95 aduh95 merged commit f759e7a into nodejs:main Feb 22, 2023
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.

Updating Web Platform Tests can cause ERR_STREAM_PREMATURE_CLOSE
6 participants