feat: add build for porting fetch to Node.js #1183

targos · 2022-01-29T08:11:43Z

In the discussion we had during the mini-summit, it was decided that we would try to include undici's fetch implementation in Node.js 18.

I just started experimenting with it and I figured it would be easier to discuss in a pull request, where we can directly try some things.

The esbuild output seems fine (I can push it here, but wanted to avoid too big of a diff), but we obviously have a problem with llhttp/WebAssembly.
Before I go too far in the wrong direction, what should we do about it? I can think of two solutions:

Adapt the part of undici that uses the llhttp parser to conditionally use the internal node binding. This is not trivial, as currently the native binding doesn't expose the exact same API that undici uses.
Inline the WASM code, like it was done for cjs-module-lexer. This seems risky because WebAssembly is not guaranteed to be available in a Node.js build.

/cc @nodejs/undici

codecov-commenter · 2022-01-29T08:14:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.73%. Comparing base (2d03e77) to head (9e782c7).
Report is 1491 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1183   +/-   ##
=======================================
  Coverage   93.73%   93.73%           
=======================================
  Files          41       43    +2     
  Lines        4020     4022    +2     
=======================================
+ Hits         3768     3770    +2     
  Misses        252      252

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

targos · 2022-01-29T08:26:58Z

Adapt the part of undici that uses the llhttp parser to conditionally use the internal node binding

If we do that, it is possible to test it in userland. Just run node --expose-internals -r internal/test/binding and internalBinding('http_parser') is available.

mcollina · 2022-01-29T08:46:09Z

Why are we bundling it? Wouldn't it be simpler to just add it to deps/ in core?

The best approach would be to use the node core bindings for http_parser. However I'm 100% happy to inline the wasm for now. I would require we use the internal binding for fetch to leave experimental.

targos · 2022-01-29T08:51:35Z

Why are we bundling it? Wouldn't it be simpler to just add it to deps/ in core?

core doesn't support relative require calls. For example, const Client = require('./lib/client') would have to be replaced with const Client = require('internal/deps/undici/lib/client').
So we need some kind of source transformation anyway, and bundling was just easier for me to experiment.

mcollina · 2022-01-29T08:58:58Z

Ah, that's true. Bundling is perfect.

targos · 2022-01-29T08:59:32Z

However I'm 100% happy to inline the wasm for now.

I see that there are two different wasm builds. Do we need to bundle both?

ronag · 2022-01-29T09:42:26Z

I think the wasm version might be faster though? Can’t we look into getting that working somehow?

targos · 2022-01-29T10:43:30Z

Should I look into inlining the WASM code for everyone or only for node core and keep the current behavior for the npm module?

ronag · 2022-01-29T10:48:19Z

I think future wise, it would be nice if Node core used a WASM build of llhttp instead of the native one in general.

It would be nice to keep the current behavior for the npm module.

mcollina · 2022-01-29T11:31:40Z

However I'm 100% happy to inline the wasm for now.

I see that there are two different wasm builds. Do we need to bundle both?

I think you can just bundle the SIMD version. However we might need both for some architecture or in case of a backport. I'm not sure if the wasm simd is available everywhere.

ronag · 2022-01-29T14:28:43Z

@targos thank you for your effort on this!

targos · 2022-01-29T14:49:07Z

Here's a Node PR based on this: nodejs/node#41749

targos · 2022-01-29T15:19:53Z

So, the current state is that both WASM builds are serialized to base64 strings which are exported from two CJS modules (58kb each). I did not keep the current behavior for the npm module because I'm not sure yet how to do it.

ronag · 2022-01-29T15:53:56Z

I’m fine with this

mcollina

lgtm

mcollina

lgtm

merceyz · 2022-01-29T16:23:25Z

Should I look into inlining the WASM code for everyone or only for node core and keep the current behavior for the npm module?

Inlining it for everyone would make it easier for other projects that also needs to bundle their dependencies.
I experimented with using Undici in Yarn and had to bundle it using babel-plugin-static-fs which stopped working in later versions of Undici. Having Undici bundle it as a base64 string itself so we don't have to manually inline it and check the bundle into our repo would be greatly appreciated!

ronag · 2022-01-29T19:12:44Z

Is this ready to merge? @targos

package.json

targos · 2022-01-29T19:56:08Z

If it's fine for you, yes!

Fixes: #19393 PR-URL: #41749 Refs: nodejs/undici#1183 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Myles Borins <[email protected]>

gr2m · 2022-02-01T18:41:13Z

Thank you. This is amazing. Thanks to everyone who made this happen 💖

miladfarca · 2022-02-04T16:06:51Z

Hello,
I just wanted to bring up a few items regarding Wasm and Simd execution.

I think you can just bundle the SIMD version.

Simd support in V8 has just been recently added to some architectures such as PPC and S390 and even then they are only supported on new hardware (i.e. PPC 9 and above). Running Simd code on older node versions or older hardware will just crash. It would be better to include both versions and fall back to regular Wasm code if Simd isn't supported.

I can see this module is is compiled with wasi-sdk: nodejs/llhttp#93
wasi-sdk doesn't seem to generate its own Javascript files to glue/call Wasm binaries and instead Js files need to be hand written per module. This could potentially cause endianness issues on big endian platforms like S390/AIX if Typed Arrays get used.

emscripten on the other hand does generate its own Js files and it's patched to handle endianness problems when dealing with Typed Arrays: emscripten-core/emscripten#13413

We are still working around some remaining problems related to its usage on BE machines but in general emscripten might be safer to use.
/cc @mhdawson

ronag · 2022-02-04T16:44:21Z

It would be better to include both versions and fall back to regular Wasm code if Simd isn't supported.

We already do this?

This could potentially cause endianness issues on big endian platforms like S390/AIX if Typed Arrays get used.

Do you have a concrete example?

PR welcome!

miladfarca · 2022-02-04T17:00:16Z

We already do this?

Thanks for confirming.

Do you have a concrete example?

Wasm memory is LE even on BE machines. Typed Arrays however follow the machine byte order.
If a Typed Array uses Wasm memory buffer it could potently read garbage data or corrupt the data on BE machines as it doesn't enforce LE ordering:
https://github.com/nodejs/llhttp/blob/main/examples/wasm.ts#L197

emscripten has been patched (link in my previous comment) to use DataViews instead of Typed Arrays which has an option to force LE ordering.

If emscripten is used then all the generated Js files should also be safe to run on BE. If Js files are being written manually then the developer should make sure Typed Arrays wont cause an endianness problem.

ronag · 2022-02-04T17:02:08Z

Wouldn't it be easier to just fix the js code in the few places where the endianess is relevant?

miladfarca · 2022-02-04T17:07:01Z

We could take that approach per project/module (i'm just using llhttp as an example here). We have to make sure no usage of typed arrays is missed and test cases cover all the scenarios otherwise random failures could happen during production runs of a given module.

ronag · 2022-02-04T17:08:49Z

In the case of undici I don't think it's that complicated?

miladfarca · 2022-02-04T17:47:43Z

Doesn't seems like its too complex. If Typed Arrays such as this: https://github.com/nodejs/llhttp/blob/main/examples/wasm.ts#L197
Are only accessing byte sized data then it should be safe to use.

Fixes: #19393 PR-URL: #41749 Refs: nodejs/undici#1183 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Myles Borins <[email protected]>

Kikobeats · 2022-02-10T10:00:39Z

Hey, I'm testing [email protected] and all is fine, just noted the original wasm files are shipped as part of the npm package:

drwxr-xr-x    - kikobeats staff 10 Feb 10:55 lib/llhttp/
.rw-r--r-- 4.2k kikobeats staff 23 Jan 17:43 ├── constants.d.ts
.rw-r--r--  11k kikobeats staff 23 Jan 17:43 ├── constants.js
.rw-r--r-- 7.0k kikobeats staff 23 Jan 17:43 ├── constants.js.map
.rwxr-xr-x  44k kikobeats staff 23 Jan 17:43 ├── llhttp.wasm*
.rw-r--r--  59k kikobeats staff  2 Feb 22:14 ├── llhttp.wasm.js
.rwxr-xr-x  44k kikobeats staff 23 Jan 17:43 ├── llhttp_simd.wasm*
.rw-r--r--  59k kikobeats staff  2 Feb 22:14 ├── llhttp_simd.wasm.js
.rw-r--r--  112 kikobeats staff 23 Jan 17:43 ├── utils.d.ts
.rw-r--r--  394 kikobeats staff 23 Jan 17:43 ├── utils.js
.rw-r--r--  432 kikobeats staff 23 Jan 17:43 └── utils.js.map

Since these files are now inline, could be possible they are no more necessary to be shipped with the library output?

that will be make undici a bit lightweight to install 🙂

mcollina · 2022-02-10T10:43:28Z

could you send a PR to remove them?

Kikobeats · 2022-02-10T11:04:07Z

@mcollina yay #1219

Fixes: nodejs#19393 PR-URL: nodejs#41749 Refs: nodejs/undici#1183 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Myles Borins <[email protected]>

Fixes: #19393 PR-URL: #41749 Backport-PR-URL: #42727 Refs: nodejs/undici#1183 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Mestery <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Myles Borins <[email protected]>

* feat: add build for porting to Node.js * inline wasm * Update package.json

Node.js core doesn't support relative require calls, so this change provides a bundles API for Postject, so that it can be used easily in Node.js. Undici too follows the same strategy. Refs: nodejs/undici#1183 Signed-off-by: Darshan Sen <[email protected]>

* feat: provide bundled API Node.js core doesn't support relative require calls, so this change provides a bundles API for Postject, so that it can be used easily in Node.js. Undici too follows the same strategy. Refs: nodejs/undici#1183 Signed-off-by: Darshan Sen <[email protected]> * fixup! test: bump timeout value from 60000 to 65000 Signed-off-by: Darshan Sen <[email protected]> * fixup! test: bump timeout value from 65000 to 70000 Signed-off-by: Darshan Sen <[email protected]> * fixup! fix: --output-api-header handling Signed-off-by: Darshan Sen <[email protected]> * fixup! refactor: bundling and copying to dist logic Signed-off-by: Darshan Sen <[email protected]> * fixup! chore: Error -> TypeError Co-authored-by: David Sanders <[email protected]> Signed-off-by: Darshan Sen <[email protected]> Co-authored-by: David Sanders <[email protected]>

* feat: add build for porting to Node.js * inline wasm * Update package.json

feat: add build for porting to Node.js

5092915

inline wasm

e4589d8

targos mentioned this pull request Jan 29, 2022

lib: add fetch nodejs/node#41749

Merged

mcollina approved these changes Jan 29, 2022

View reviewed changes

targos commented Jan 29, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

Update package.json

9e782c7

ronag merged commit 19b5789 into nodejs:main Jan 30, 2022

targos deleted the build-node branch January 30, 2022 09:49

ef4 mentioned this pull request Jan 31, 2022

Rename import path to ember-fetch ember-cli/ember-fetch#330

Open

mcfedr mentioned this pull request Feb 4, 2022

Fix order of WASM loading #1204

Merged

Kikobeats mentioned this pull request Feb 10, 2022

chore: ignore wasm file for npm publishing #1219

Merged

ef4 mentioned this pull request Feb 16, 2022

Archive this repo? ember-cli/ember-fetch#656

Open

KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022

feat: add build for porting fetch to Node.js (nodejs#1183)

357a3c4

* feat: add build for porting to Node.js * inline wasm * Update package.json

RaisinTen mentioned this pull request Oct 10, 2022

feat: provide bundled API nodejs/postject#49

Merged

RaisinTen mentioned this pull request Oct 27, 2022

deps: vendor semver nodejs/node#45127

Open

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022

feat: add build for porting fetch to Node.js (nodejs#1183)

827a973

* feat: add build for porting to Node.js * inline wasm * Update package.json

KhafraDev mentioned this pull request Oct 19, 2023

Node Version 18.18.2 Fails to Instantiate Fetch Request with Proper Options nodejs/node#50263

Closed

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

feat: add build for porting fetch to Node.js (nodejs#1183)

9ba5f41

* feat: add build for porting to Node.js * inline wasm * Update package.json

staltz mentioned this pull request Sep 30, 2024

Usage of fetch on iOS causes crash due to no WebAssembly nodejs-mobile/nodejs-mobile#71

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add build for porting fetch to Node.js #1183

feat: add build for porting fetch to Node.js #1183

targos commented Jan 29, 2022 •

edited

Loading

codecov-commenter commented Jan 29, 2022 •

edited

Loading

targos commented Jan 29, 2022

mcollina commented Jan 29, 2022

targos commented Jan 29, 2022 •

edited

Loading

mcollina commented Jan 29, 2022

targos commented Jan 29, 2022

ronag commented Jan 29, 2022

targos commented Jan 29, 2022 •

edited

Loading

ronag commented Jan 29, 2022 •

edited

Loading

mcollina commented Jan 29, 2022

ronag commented Jan 29, 2022

targos commented Jan 29, 2022

targos commented Jan 29, 2022

ronag commented Jan 29, 2022

mcollina left a comment

mcollina left a comment

merceyz commented Jan 29, 2022 •

edited

Loading

ronag commented Jan 29, 2022

targos commented Jan 29, 2022

gr2m commented Feb 1, 2022

miladfarca commented Feb 4, 2022 •

edited

Loading

ronag commented Feb 4, 2022 •

edited

Loading

miladfarca commented Feb 4, 2022 •

edited

Loading

ronag commented Feb 4, 2022

miladfarca commented Feb 4, 2022

ronag commented Feb 4, 2022

miladfarca commented Feb 4, 2022

Kikobeats commented Feb 10, 2022

mcollina commented Feb 10, 2022

Kikobeats commented Feb 10, 2022

feat: add build for porting fetch to Node.js #1183

feat: add build for porting fetch to Node.js #1183

Conversation

targos commented Jan 29, 2022 • edited Loading

codecov-commenter commented Jan 29, 2022 • edited Loading

Codecov Report

targos commented Jan 29, 2022

mcollina commented Jan 29, 2022

targos commented Jan 29, 2022 • edited Loading

mcollina commented Jan 29, 2022

targos commented Jan 29, 2022

ronag commented Jan 29, 2022

targos commented Jan 29, 2022 • edited Loading

ronag commented Jan 29, 2022 • edited Loading

mcollina commented Jan 29, 2022

ronag commented Jan 29, 2022

targos commented Jan 29, 2022

targos commented Jan 29, 2022

ronag commented Jan 29, 2022

mcollina left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

merceyz commented Jan 29, 2022 • edited Loading

ronag commented Jan 29, 2022

targos commented Jan 29, 2022

gr2m commented Feb 1, 2022

miladfarca commented Feb 4, 2022 • edited Loading

ronag commented Feb 4, 2022 • edited Loading

miladfarca commented Feb 4, 2022 • edited Loading

ronag commented Feb 4, 2022

miladfarca commented Feb 4, 2022

ronag commented Feb 4, 2022

miladfarca commented Feb 4, 2022

Kikobeats commented Feb 10, 2022

mcollina commented Feb 10, 2022

Kikobeats commented Feb 10, 2022

targos commented Jan 29, 2022 •

edited

Loading

codecov-commenter commented Jan 29, 2022 •

edited

Loading

targos commented Jan 29, 2022 •

edited

Loading

targos commented Jan 29, 2022 •

edited

Loading

ronag commented Jan 29, 2022 •

edited

Loading

merceyz commented Jan 29, 2022 •

edited

Loading

miladfarca commented Feb 4, 2022 •

edited

Loading

ronag commented Feb 4, 2022 •

edited

Loading

miladfarca commented Feb 4, 2022 •

edited

Loading