Fix Dynamic require of net crash in the published Action - #15
Merged
Conversation
Real bug reported from live usage in another repo: the Action failed
immediately on every run with
Error: Dynamic require of "net" is not supported
at .../tunnel/lib/tunnel.js
at .../tunnel/index.js
esbuild's ESM output has no real `require` global, so it injects a
shim (`typeof require !== "undefined" ? require : ... throw`) for any
require() call it can't statically resolve. The `tunnel` package
(pulled in transitively via @actions/http-client's proxy support,
itself a dependency of octokit) does `require('net')`/`require('tls')`
etc. at its own top level, hits that shim, finds no real `require`,
and throws - immediately on module load, before any of our own code
runs.
First attempt was switching the bundle to CJS output (where require
is real), but that broke something else: esbuild empties out
import.meta.url entirely under CJS ("will be empty" warning), and
@docmend/core's wasm-path resolution (grammar-loader.ts, fixed for
the original bundling bug back in the publish-prep phase) depends on
import.meta.url to find the wasm files sitting next to the bundle.
Trading one crash for a silent one.
Fixed properly by staying on ESM output and instead giving the
bundle's shim a genuine `require` to find, via esbuild's own
documented banner-injection fix for exactly this class of error:
import { createRequire as X } from 'module';
const require = X(import.meta.url);
Aliased the createRequire import specifically because grammar-loader.ts
already imports createRequire under its own name and gets bundled
into the same module scope - importing it twice unaliased is a
duplicate declaration (a real SyntaxError I hit and fixed before
landing on this).
Consolidated the whole build step (previously a long inline esbuild
CLI invocation, awkward to correctly quote a banner flag inside a
package.json script string) into scripts/build.mjs using esbuild's
JS API instead, which also copies the wasm files as before.
Also bumped runs.using from 'node20' to 'node24' in action.yml -
the same failing run's log included GitHub's own deprecation notice
that Node 20 is being phased out and runners already default to
Node 24 regardless of what the action declares.
Verified with the same rigor as the original wasm-bundling fix, not
just "no warnings": reproduced the exact original crash in a clean,
zero-node_modules directory (confirmed the unaliased-banner attempt
still failed, this time with a different, real SyntaxError - caught
before considering this done), then confirmed the final build loads
successfully in that same clean isolation and that real WASM-based
code parsing still works correctly there.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Real bug from live usage, reported after trying
dakshcodez/docmend@v1in another repo - the Action failed immediately on every run:esbuild's ESM output has no real
requireglobal, so it shims anyrequire()call it can't statically resolve - and thetunnelpackage (pulled in transitively via@actions/http-client's proxy support, itself an octokit dependency) callsrequire('net')at its own top level, hits the shim, finds nothing real, throws immediately on module load.What was tried and what actually worked
First attempt: switch the bundle to CJS output (real
requireexists there). This introduced a different bug instead - esbuild empties outimport.meta.urlentirely under CJS, and@docmend/core's wasm-path resolution depends on it. Would have traded a loud crash for silent wasm-loading failure.Fixed properly by staying on ESM output and giving the bundle's shim a genuine
requirevia esbuild's own documented banner-injection fix for this exact error class:The
createRequireimport needed aliasing specifically becausegrammar-loader.tsalready imports it under its own name and gets bundled into the same module scope - the unaliased version hit a realSyntaxError(duplicate declaration) during testing, caught before landing on this.Also consolidated the build step into
scripts/build.mjsusing esbuild's JS API (the banner flag was awkward to correctly shell-quote as an inlinepackage.jsonscript string), and bumpedruns.usingfromnode20tonode24- the same failing run's log included GitHub's deprecation notice that Node 20 is being phased out.Test plan
node_modulesisolated directorySyntaxError) before landing on the working fix - not just accepting the first thing that seemed to worknpm run lint/typecheck/buildall pass