Allow offline run of build:node script

[khardix]

This relates to...

#2342

Rationale

During packaging for a Linux distribution, the build systems are usually/sometimes disconnected from the internet,
and can not use anything not present in source tarball or installed on the system.

To facilitate NodeJS packages, in Fedora we cache the node_modules directory when creating a package update, and then re-use this cached directory for all subsequent builds. That means that we need all the build-time dependencies installed after invoking npm install --include=dev. This is currently not true for the build:node script, which pulls esbuild@0.19.4 at the point of invocation. If invoked in offline mode (npm --offline run build:node), this fails.

Changes

• Include esbuild in devDependencies, so that it is installed on npm install --include=dev.
• Use the installed version when running the build:node script.

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• [S] Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[khardix]

Note: I'm not that versed in invocation of npx, so if there is a better way to achieve what I need, let me know.

[codecov-commenter]

Codecov Report

Attention: 197 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (7d8241b) 84.97%.
Report is 232 commits behind head on main.

Files	Patch %	Lines
lib/fetch/index.js	67.29%	52 Missing ⚠️
lib/fetch/util.js	37.17%	49 Missing ⚠️
lib/cache/cache.js	8.33%	33 Missing ⚠️
lib/handler/RetryHandler.js	74.35%	30 Missing ⚠️
lib/core/diagnostics.js	84.74%	9 Missing ⚠️
lib/api/readable.js	88.88%	5 Missing ⚠️
lib/fetch/headers.js	90.19%	5 Missing ⚠️
lib/client.js	95.23%	3 Missing ⚠️
lib/compat/dispatcher-weakref.js	57.14%	3 Missing ⚠️
lib/core/util.js	95.65%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2633      +/-   ##
==========================================
- Coverage   85.54%   84.97%   -0.58%     
==========================================
  Files          76       81       +5     
  Lines        6858     7290     +432     
==========================================
+ Hits         5867     6195     +328     
- Misses        991     1095     +104     

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

[mhdawson]

@KhafraDev since I think the use of npx was introduced in #2342.

This seems like a reasonable tweak to handle the use case outlined by @khardix.

[KhafraDev]

It was added by @targos in 946ea78

The description may apply here (especially w/ nodejs/citgm#982)?:

It breaks AIX in CITGM. Use npx instead.

[khardix]

Hm, no idea what AIX is, but your comment tells me this was a deliberate change and not easily reverted. Since I can patch this downstream without too much of an effort, I'm withdrawing this PR. Thanks for the explanation!

[mhdawson]

@khardix AIX is one of the IBM platforms we help maintain in the upstream. I don't quite understand why one invocation of npx would work but not the other one. I believe that esbuild more recently started supporting AIX.

I think do think its worth figuring out. Can you re-open the issue until we do that. We probably want to understand what the issue was on AIX anyway and if its no longer an issue, one less patch upstream is better.

@abmusse can you confirm what I said about esbuild being recently update to support AIX? And possibly take a look to help us understand why one invocation works while the other does not. It might be an issue on AIX we at least want to be aware of.

[targos]

It's not that one invocation works while the other does not. It's that the npm install command failed on the AIX platform when esbuild was a dependency.
This particular command is not used to run undici test suite.

[KhafraDev]

It is, we test the node bundle in the fetch suite.

[targos]

Maybe it wasn't back in 2022

[richardlau]

It's not that one invocation works while the other does not. It's that the npm install command failed on the AIX platform when esbuild was a dependency. This particular command is not used to run undici test suite.

It used to be the case that esbuild didn't provide binaries for AIX but as of evanw/esbuild#3550 and esbuild@0.19.10 it does.
Refs: evanw/esbuild#3549

[abmusse]

@abmusse can you confirm what I said about esbuild being recently update to support AIX? And possibly take a look to help us understand why one invocation works while the other does not. It might be an issue on AIX we at least want to be aware of

ditto with Richard's comment above #2633 (comment)

Version 0.19.10 added pre-complied binaries for AIX. I tested installing esbuild on an AIX machine and that worked.

evanw/esbuild#3549 (comment)

Comprehensive testing was not done though.

[mhdawson]

From my perspective if we can somehow test that CITGM will run, then that would be best and would confirm that this change would be ok.