Thanks for the report, I'll take look into it.

This is one of the issues I'm hoping a Fedora CI will help prevent, once the open discussions there are resolved (and the tests are enabled): https://gitlab.com/redhat/centos-stream/tests/nodejs

Tried it in the container (registry.fedoraproject.org/fedora:rawhide):

> # node --expose-internals -e 'const {default: undici} = import("node:internal/deps/undici/undici");'
> node:internal/bootstrap/realm:431
>   if (!mod) throw new TypeError(`Missing internal module '${id}'`);
>                   ^
>
> TypeError: Missing internal module 'internal/deps/./lib/client'
>     at requireBuiltin (node:internal/bootstrap/realm:431:19)
>     at requireWithFallbackInDeps (node:internal/bootstrap/realm:443:10)
>     at node:internal/deps/undici/undici:3:16
>     at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/realm:401:7)
>     at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/realm:337:10)
>     at loadBuiltinModule (node:internal/modules/helpers:101:7)
>     at ModuleLoader.builtinStrategy (node:internal/modules/esm/translators:452:18)
>     at callTranslator (node:internal/modules/esm/loader:285:14)
>     at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:291:30)
>     at async link (node:internal/modules/esm/module_job:76:21)
>
> Node.js v20.11.0

It looks to me that when undici (index.js) tries to import it's `lib/client`, it cannot find it; that file exists, but I guess the correct search path should be `internal/deps/undici/lib/client` (a directory below where it looks now).

Further observation: I'm packaging undici differently than upstream nodejs imports it into it's source. Upstream does the following (from `tools/dep_updaters/update-undici.sh`):

> "$NODE" "$NPM" install --no-bin-link --ignore-scripts
> "$NODE" "$NPM" run build:node
> "$NODE" "$NPM" prune --production
...
> mv deps/undici/src/undici-fetch.js deps/undici/undici.js

The problem is that the `undici-fetch.js` later used as "the module" for the externalized builtin is generated via the `build:node` script, which invokes `npx esbuild.4 …`. Esbuild is not listed as a undici dependency, thus is not available for our build.

IIRC I tried to workaround it by simply not running the esbuild, instead pointing the `--shared-builtin` flag to undici's `index.js`. That seemed to work, but not anymore.

---

The more future-proof version would probably be to add the esbuild to dev dependencies and then following the node updater script – that should give us parity with whatever the upstream project expects. However, I would still be interested in what changed between the nodejs versions and why the relative imports stopped working.

Seems like there was a collision when I posted this earlier:

I'm testing out the Node.js upstream following these steps

1) clone https://github.com/nodejs/node.git
2) checkout out the latest v20.x (git checkout v20.x)
3) copied deps/undici/undici.js to a different directory and name
4) configure with ./configure --shared-builtin-undici/undici-path=/newpull/io.js/undici2.js
5) built/ran full test suite with make test

Seemed to built/pass tests.

Validated that it was using the externalized version of undici by renaming undici2.js and running "./node" and it failed as expected with a message that the externalized builtin could not be found

I further validated that I can build/test having removed undici under the deps completely in the Node.js project, so there is no change in the `Node.js` project.

From the comments above I can see that what likely changed was how undici is built, which caused the build of the undici package to not package correctly.

I think this is likely related - https://github.com/nodejs/undici/pull/2342/, it removed `esbuild` as a dep and then used `npx to run esbuild` instead.


> The more future-proof version would probably be to add the esbuild to dev dependencies and then following the node updater script – that should give us parity with whatever the upstream project expects. However, I would still be interested in what changed between the nodejs versions and why the relative imports stopped working.

I agree that using the updater script it the best way to ensuring things keep in sync.

> IIRC I tried to workaround it by simply not running the esbuild, instead pointing the `--shared-builtin` flag to undici's `index.js`. That seemed to work, but not anymore.


I think that might also have had the side effect of exposing more from undici than is done in Node.js.

I have proposed adding the necessary bundler to devDependencies upstream: https://github.com/nodejs/undici/pull/2633; it was turned down since it apparently breaks another project. We do not have that problem, so I'm modifying the `undici-sources.sh` to add the esbuild to devDependencies, so that I can recreate the correct file while building the RPM. PR incoming once I test locally that it works.

Are we fine with (only) a bundled .js file in the final package?

I'm working on this, and Node doesn't seem to like externalized builtins importing more externalized paths, since it is loading the externalized module as a string and doesn't forward nor reference it by its path (src/node_builtins.cc:BuiltinLoader::AddExternalizedBuiltin).

So we could:
a) only ship the bundled version within the package
   (same as upstream, but guaranteed to break users who use `require('undici')` or any subpaths without a local installation of undici)
b) use a custom loader as the entrypoint to load the unbundled version in /usr/lib/node_modules
   (could have some unknown quirks, which I'm not certain it will happen or not.
    If so, it's most likely due to the hardcoded path in the loader,
    since we have to somehow reference the module without using relative paths)
c) ship the bundled version and the unbundled version at the same time
   (impossible to break anything but comes with 2x space usage)

I prefer (b), since:
- imo it's cleaner than (a)
- the error trace actually maps to the original source
- plus, we don't need to add esbuild to BR

Either choosing (b) or (c) requires a rebuild on nodejs.


---

A custom loader poc:

undici-fetch.js
```javascript
'use strict';

const moduleRoot = "%{nodejs_sitelib}/%{npm_name}"; // should be expanded to real path when building
module.exports = require(moduleRoot);
```

*edit: the previous `moduleRoot` should be `moduleRoot + '/index-fetch.js'`, since it's a different entrypoint being built in the bundle.

I also wonder why they used WASM instead of a NAPI extension for llhttp in such a performance-sensitive context. (it even came with a SIMD vector-optimized build!)
Native is approximately 1.75x-2.5x faster than the WASM implementation on the same codebase.

(In reply to Jan Staněk from comment #7)
> I have proposed adding the necessary bundler to devDependencies upstream:
> https://github.com/nodejs/undici/pull/2633; it was turned down since it
> apparently breaks another project. We do not have that problem, so I'm
> modifying the `undici-sources.sh` to add the esbuild to devDependencies, so
> that I can recreate the correct file while building the RPM. PR incoming
> once I test locally that it works.

W.r.t. https://github.com/nodejs/undici/pull/2633 the "breaks another project" was that previously esbuild didn't have pre-compiled binaries for AIX (an IBM OS) (https://github.com/evanw/esbuild/issues/3549) which meant that npm installs of undici failed citgm runs in upstream Node.js. As of esbuild.10 (and https://github.com/evanw/esbuild/pull/3550) that has been rectified so undici could possibly go back to declaring esbuild as a devDependency again without breaking citgm runs in upstream Node.js.

I propose this fix:

rpms/nodejs20:
diff --git a/nodejs20.spec b/nodejs20.spec
index f1f9fa6..786c302 100644
--- a/nodejs20.spec
+++ b/nodejs20.spec
@@ -547,7 +547,7 @@ export PATH="${cwd}/.bin:$PATH"
            %{!?with_bundled_zlib:--shared-zlib} \
            %{!?with_bootstrap:--shared-builtin-cjs_module_lexer/lexer-path %{nodejs_default_sitelib}/cjs-module-lexer/lexer.js} \
            %{!?with_bootstrap:--shared-builtin-cjs_module_lexer/dist/lexer-path %{nodejs_default_sitelib}/cjs-module-lexer/dist/lexer.js} \
-           %{!?with_bootstrap:--shared-builtin-undici/undici-path %{nodejs_default_sitelib}/undici/index.js} \
+           %{!?with_bootstrap:--shared-builtin-undici/undici-path %{nodejs_default_sitelib}/undici/loader.js} \
            --shared-brotli \
            --shared-libuv \
            --with-intl=small-icu \



/usr/lib/node_modules/undici/loader.js:
```javascript
'use strict';

module.exports = require('node:module').createRequire('/usr/lib/node_modules/undici/loader.js')('./index-fetch.js');
```

(In reply to Zephyr Lykos from comment #12)
> I propose this fix:
> 
> rpms/nodejs20:
> diff --git a/nodejs20.spec b/nodejs20.spec
> index f1f9fa6..786c302 100644
> --- a/nodejs20.spec
> +++ b/nodejs20.spec
> @@ -547,7 +547,7 @@ export PATH="${cwd}/.bin:$PATH"
>             %{!?with_bundled_zlib:--shared-zlib} \
>             %{!?with_bootstrap:--shared-builtin-cjs_module_lexer/lexer-path
> %{nodejs_default_sitelib}/cjs-module-lexer/lexer.js} \
>            
> %{!?with_bootstrap:--shared-builtin-cjs_module_lexer/dist/lexer-path
> %{nodejs_default_sitelib}/cjs-module-lexer/dist/lexer.js} \
> -           %{!?with_bootstrap:--shared-builtin-undici/undici-path
> %{nodejs_default_sitelib}/undici/index.js} \
> +           %{!?with_bootstrap:--shared-builtin-undici/undici-path
> %{nodejs_default_sitelib}/undici/loader.js} \
>             --shared-brotli \
>             --shared-libuv \
>             --with-intl=small-icu \
> 
> 
> 
> /usr/lib/node_modules/undici/loader.js:
> ```javascript
> 'use strict';
> 
> module.exports =
> require('node:module').createRequire('/usr/lib/node_modules/undici/loader.
> js')('./index-fetch.js');
> ```

As I wrote on the PR (sidenote: Should really start to read all the emails before replying.), I have nothing against the idea in general; however, I'm not comfortable with maintaining an (yet another unsupported) approach different from upstream.

Rigth now, I'm working on:
1. Downstream application of the "move esbuild to devDependencies"; makes us not depend on the upstream conflict, and will be removed in favor of upstream if anyone pushes the change through.
2. Run the esbuild and include the extra file in the RPM (the (c) solution from above). Although wasting space, it will be as close to upstream as possible, which should minimize chance of breakage.

Any other changes I need to see at least discussed upstream before entertaining the idea of maintaining them separately.

TL;DR: We should package only the bare minimum essentials that differ from how upstream handles things; if that is not optimal/pretty/could be done better, try to improve the upstream way of doing things first.

---

To be clear, I'm glad that there is activity and the proposal looks very good (or at least interesting) to me. Kudos to you. :-)

So, my proposed fixes, which should be as minimal as possible (i.e. no updates for undici yet):

nodejs-undici: https://src.fedoraproject.org/rpms/nodejs-undici/pull-request/4# - Moves esbuild to devDependencies, uses that to create undici-fetch.js during build.
nodejs20: https://src.fedoraproject.org/rpms/nodejs20/pull-request/6 – Utilizes the undici-fetch.js from ^; needs that to land first.

Rawhide is currently a bit wonky for me; I cannot seem to be able to build nodejs20 due to some issues with annobin/GCC 14. I managed to get it working in F39 chroot, though.

---

Personal note: Although I'm very glad I have the attention of others on this and related issues, I feel a bit overwhelmed right now with all NodeJS work I have on my plate. Apologies in case I'm stepping on anybody's toes without realising; it's not intentional.

FEDORA-2024-ff9e7b78fc (nodejs20-20.11.0-4.fc40) has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-ff9e7b78fc

FEDORA-2024-ff9e7b78fc (nodejs20-20.11.0-4.fc40) has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.