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.

Sign up for GitHub

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

Jump to bottom

undici.fetch breaks on Node.js <=18.13.0 in undici@6.13.0 #3123

Closed
trentm opened this issue Apr 15, 2024 · 10 comments
Closed

undici.fetch breaks on Node.js <=18.13.0 in undici@6.13.0 #3123

trentm opened this issue Apr 15, 2024 · 10 comments
Labels
bug Something isn't working

Comments

@trentm
Copy link
Contributor

trentm commented Apr 15, 2024

Bug Description

Since undici@6.13.0, undici.fetch breaks on Node.js 18.13.0 and earlier.
"package.json" includes:

  "engines": {
    "node": ">=18.0"
  },

but I'm not sure if anything less than v18.17.0 is really supported anymore -- the move to borp as part of #2267 means that the test suite cannot be run with Node.js earlier than v18.17.0.

Reproducible By

Using this "playfetch.js":

% cat playfetch.js
const undici = require('./');
async function main() {
  const res = await undici.fetch('http://www.google.com/');
  console.log('res:', res.status);
  const text = await res.text();
  console.log('text: %s...', text.slice(0, 70));
}
main();

In a git clone at commit 8a07bbd (from #3095):

% git remote -v
origin	git@github.com:nodejs/undici.git (fetch)
origin	git@github.com:nodejs/undici.git (push)

% git log1 -1
* 8a07bbd7 - (HEAD) fetch: avoid creation of an intermediary ReadableStream (#3095) (4 days ago) <Khafra>

% node --version
v18.13.0

% node playfetch.js
res: 200
/Users/trentm/src/undici/lib/web/fetch/index.js:2034
        fetchParams.controller.controller.error(new TypeError('terminated', {
                                                ^

TypeError: terminated
    at Fetch.onAborted (/Users/trentm/src/undici/lib/web/fetch/index.js:2034:49)
    at Fetch.emit (node:events:513:28)
    at Fetch.terminate (/Users/trentm/src/undici/lib/web/fetch/index.js:93:10)
    at /Users/trentm/src/undici/lib/web/fetch/index.js:506:30
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  [cause]: TypeError [ERR_INVALID_ARG_TYPE]: The "stream" argument must be an instance of Stream. Received an instance of ReadableStream
      at new NodeError (node:internal/errors:400:5)
      at eos (node:internal/streams/end-of-stream:66:11)
      at fetchFinale (/Users/trentm/src/undici/lib/web/fetch/index.js:1093:5)
      at mainFetch (/Users/trentm/src/undici/lib/web/fetch/index.js:760:5)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
    code: 'ERR_INVALID_ARG_TYPE'
  }
}

Node.js v18.13.0

This passes in newer Node.js versions.
This passes in earlier undici commits.

Expected Behavior

That "playfetch.js" show work, e.g. with more recent Node v18:

% node --version
v18.20.1

% node playfetch.js
res: 200
text: <!doctype html><html itemscope="" itemtype="http://schema.org/WebPage"...

Environment

Tested on macOS 14.4.1, x86_64.
@trentm trentm added the bug Something isn't working label Apr 15, 2024
@trentm trentm mentioned this issue Apr 15, 2024
Merged
@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 15, 2024

Why not upgrading node version too?

@KhafraDev
Copy link
Member

Why not upgrading node version too?

This isn't relevant.

This is caused by 8a07bbd which uses stream.finished. The node docs did not mention that support for web streams was added in some unknown v18 release ☹️.

The fix is either to drop support for v18 prior to said addition, or fallback to the previous solution if we can't use stream.finished.

This was referenced Apr 15, 2024
Closed
Merged
@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 15, 2024

It is relevant for the decision if we drop support for node versions < 18.17.0 or if we fix the support for node 18.14.0. In fastify we expect that a reasonable version of node has to be used. so nobody can demand, that we support node 14.0.0, but we support latest node 14.

@KhafraDev
Copy link
Member

node 14 is EOL, node 18 isn't

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 15, 2024

it is the principle. We also dont support older versions of node 18 or 20, despite them being not EOL:

@KhafraDev
Copy link
Member

That's not true, we currently support any version of node >= v18:

undici/package.json

Lines 127 to 129 in f979ec3

"engines": {
"node": ">=18.0"
},

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 15, 2024

I was in the context of fastify.

@mcollina
Copy link
Member

I think it's safe to say we support only recent versions of Node.js and we didn't update package.json.

Mostly because we do not test for them.
I generically follow the rule that only the latest version of a series is supported, mostly because it's impossible work to test them all and we should be encouraging people to upgrade

@KhafraDev
Copy link
Member

Please downgrade undici to v6.12.0 or upgrade your node version to the latest version of v18.

@trentm
Copy link
Contributor Author

trentm commented Apr 16, 2024

Okay, thanks. To be clear, I don't need undici to support the older Node v18 versions for my case. I just needed to be clear what versions it did support.
It would be helpful if "engines" was updated to a realistic value, but perhaps that is difficult if the intent will be to only support the very latest.

trentm added a commit to elastic/apm-agent-nodejs that referenced this issue Apr 16, 2024
@dependabot @trentm
chore(deps-dev): bump undici from 6.11.1 to 6.13.0 (#3975)
8dec118 
test(undici): reduce testing of undici@6 on older Node.js v18 versions because of undici breakage

Starting with undici@6.13.0, it breaks on Node.js 18.13.0 and earlier.
Undici somewhat officially only supports the latest minor version of supported
Node.js majors, so we'll bump our testing to only more recent Node.js v18 versions.

Refs: nodejs/undici#3123
Co-authored-by: Trent Mick <trent.mick@elastic.co>
david-luna pushed a commit to elastic/apm-agent-nodejs that referenced this issue Apr 24, 2024
@dependabot @trentm @david-luna
chore(deps-dev): bump undici from 6.11.1 to 6.13.0 (#3975)
daacf75 
test(undici): reduce testing of undici@6 on older Node.js v18 versions because of undici breakage

Starting with undici@6.13.0, it breaks on Node.js 18.13.0 and earlier.
Undici somewhat officially only supports the latest minor version of supported
Node.js majors, so we'll bump our testing to only more recent Node.js v18 versions.

Refs: nodejs/undici#3123
Co-authored-by: Trent Mick <trent.mick@elastic.co>
@ivov ivov mentioned this issue May 29, 2024
Merged
@timrogers timrogers mentioned this issue Jun 27, 2024
Merged
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this issue Aug 20, 2024
@dependabot @trentm @fpm-peter
chore(deps-dev): bump undici from 6.11.1 to 6.13.0 (elastic#3975)
fdeba1f 
test(undici): reduce testing of undici@6 on older Node.js v18 versions because of undici breakage

Starting with undici@6.13.0, it breaks on Node.js 18.13.0 and earlier.
Undici somewhat officially only supports the latest minor version of supported
Node.js majors, so we'll bump our testing to only more recent Node.js v18 versions.

Refs: nodejs/undici#3123
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix fetch support for node < 18.14.0 KhafraDev/undici
4 participants
@trentm @mcollina @Uzlopak @KhafraDev