Work around issues with Deno 1.31+ (#3685)

evanw · web-flow · commit 116f63ec1d06 · 2024-03-07T17:32:05.000-05:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,6 +35,16 @@
     })(Foo || {});
     ```
 
+* Work around issues with Deno 1.31+ ([#3682](https://github.com/evanw/esbuild/issues/3682))
+
+    Version 0.20.0 of esbuild changed how the esbuild child process is run in esbuild's API for Deno. Previously it used `Deno.run` but that API is being removed in favor of `Deno.Command`. As part of this change, esbuild is now calling the new `unref` function on esbuild's long-lived child process, which is supposed to allow Deno to exit when your code has finished running even though the child process is still around (previously you had to explicitly call esbuild's `stop()` function to terminate the child process for Deno to be able to exit).
+
+    However, this introduced a problem for Deno's testing API which now fails some tests that use esbuild with `error: Promise resolution is still pending but the event loop has already resolved`. It's unclear to me why this is happening. The call to `unref` was recommended by someone on the Deno core team, and calling Node's equivalent `unref` API has been working fine for esbuild in Node for a long time. It could be that I'm using it incorrectly, or that there's some reference counting and/or garbage collection bug in Deno's internals, or that Deno's `unref` just works differently than Node's `unref`. In any case, it's not good for Deno tests that use esbuild to be failing.
+
+    In this release, I am removing the call to `unref` to fix this issue. This means that you will now have to call esbuild's `stop()` function to allow Deno to exit, just like you did before esbuild version 0.20.0 when this regression was introduced.
+
+    Note: This regression wasn't caught earlier because Deno doesn't seem to fail tests that have outstanding `setTimeout` calls, which esbuild's test harness was using to enforce a maximum test runtime. Adding a `setTimeout` was allowing esbuild's Deno tests to succeed. So this regression doesn't necessarily apply to all people using tests in Deno.
+
 ## 0.20.1
 
 * Fix a bug with the CSS nesting transform ([#3648](https://github.com/evanw/esbuild/issues/3648))
diff --git a/lib/deno/mod.ts b/lib/deno/mod.ts
@@ -192,8 +192,6 @@ type SpawnFn = (cmd: string, options: {
   read(): Promise<Uint8Array | null>
   close(): Promise<void> | void
   status(): Promise<{ code: number }>
-  unref(): void
-  ref(): void
 }
 
 // Deno ≥1.40
@@ -235,8 +233,6 @@ const spawnNew: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => {
       await child.status
     },
     status: () => child.status,
-    unref: () => child.unref(),
-    ref: () => child.ref(),
   }
 }
 
@@ -277,8 +273,6 @@ const spawnOld: SpawnFn = (cmd, { args, stdin, stdout, stderr }) => {
       child.close()
     },
     status: () => child.status(),
-    unref: () => { },
-    ref: () => { },
   }
 }
 
@@ -334,20 +328,12 @@ const ensureServiceIsRunning = (): Promise<Service> => {
       })
       readMoreStdout()
 
-      let refCount = 0
-      child.unref() // Allow Deno to exit when esbuild is running
-
-      const refs: common.Refs = {
-        ref() { if (++refCount === 1) child.ref(); },
-        unref() { if (--refCount === 0) child.unref(); },
-      }
-
       return {
         build: (options: types.BuildOptions) =>
           new Promise<types.BuildResult>((resolve, reject) => {
             service.buildOrContext({
               callName: 'build',
-              refs,
+              refs: null,
               options,
               isTTY,
               defaultWD,
@@ -359,7 +345,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
           new Promise<types.BuildContext>((resolve, reject) =>
             service.buildOrContext({
               callName: 'context',
-              refs,
+              refs: null,
               options,
               isTTY,
               defaultWD,
@@ -370,7 +356,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
           new Promise<types.TransformResult>((resolve, reject) =>
             service.transform({
               callName: 'transform',
-              refs,
+              refs: null,
               input,
               options: options || {},
               isTTY,
@@ -403,7 +389,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
           new Promise((resolve, reject) =>
             service.formatMessages({
               callName: 'formatMessages',
-              refs,
+              refs: null,
               messages,
               options,
               callback: (err, res) => err ? reject(err) : resolve(res!),
@@ -413,7 +399,7 @@ const ensureServiceIsRunning = (): Promise<Service> => {
           new Promise((resolve, reject) =>
             service.analyzeMetafile({
               callName: 'analyzeMetafile',
-              refs,
+              refs: null,
               metafile: typeof metafile === 'string' ? metafile : JSON.stringify(metafile),
               options,
               callback: (err, res) => err ? reject(err) : resolve(res!),
diff --git a/lib/shared/types.ts b/lib/shared/types.ts
@@ -664,13 +664,15 @@ export let version: string
 
 // Call this function to terminate esbuild's child process. The child process
 // is not terminated and re-created after each API call because it's more
-// efficient to keep it around when there are multiple API calls. This child
-// process normally exits automatically when the parent process exits, so you
-// usually don't need to call this function.
+// efficient to keep it around when there are multiple API calls.
 //
-// One reason you might want to call this is if you know you will not make any
-// more esbuild API calls and you want to clean up resources (since the esbuild
-// child process takes up some memory even when idle).
+// In node this happens automatically before the parent node process exits. So
+// you only need to call this if you know you will not make any more esbuild
+// API calls and you want to clean up resources.
+//
+// Unlike node, Deno lacks the necessary APIs to clean up child processes
+// automatically. You must manually call stop() in Deno when you're done
+// using esbuild or Deno will continue running forever.
 //
 // Another reason you might want to call this is if you are using esbuild from
 // within a Deno test. Deno fails tests that create a child process without
diff --git a/scripts/deno-tests.js b/scripts/deno-tests.js
@@ -15,22 +15,20 @@ try {
 Deno.mkdirSync(rootTestDir, { recursive: true })
 
 function test(name, backends, fn) {
-  const singleTest = (name, fn) => Deno.test({
-    name,
-    fn: () => new Promise((resolve, reject) => {
-      const minutes = 5
-      const timeout = setTimeout(() => reject(new Error(`Timeout for "${name}" after ${minutes} minutes`)), minutes * 60 * 1000)
-      const cancel = () => clearTimeout(timeout)
-      const promise = fn()
-      promise.then(cancel, cancel)
-      promise.then(resolve, reject)
-    }),
-  })
+  // Note: Do not try to add a timeout for the tests below. It turns out that
+  // calling "setTimeout" from within "Deno.test" changes how Deno waits for
+  // promises in a way that masks problems that would otherwise occur. We want
+  // test coverage for the way that people will likely be using these API calls.
+  //
+  // Specifically tests that Deno would otherwise have failed with "error:
+  // Promise resolution is still pending but the event loop has already
+  // resolved" were being allowed to pass instead. See this issue for more
+  // information: https://github.com/evanw/esbuild/issues/3682
 
   for (const backend of backends) {
     switch (backend) {
       case 'native':
-        singleTest(name + '-native', async () => {
+        Deno.test(name + '-native', async () => {
           let testDir = path.join(rootTestDir, name + '-native')
           await Deno.mkdir(testDir, { recursive: true })
           try {
@@ -43,7 +41,7 @@ function test(name, backends, fn) {
         break
 
       case 'wasm-main':
-        singleTest(name + '-wasm-main', async () => {
+        Deno.test(name + '-wasm-main', async () => {
           let testDir = path.join(rootTestDir, name + '-wasm-main')
           await esbuildWASM.initialize({ wasmModule, worker: false })
           await Deno.mkdir(testDir, { recursive: true })
@@ -57,7 +55,7 @@ function test(name, backends, fn) {
         break
 
       case 'wasm-worker':
-        singleTest(name + '-wasm-worker', async () => {
+        Deno.test(name + '-wasm-worker', async () => {
           let testDir = path.join(rootTestDir, name + '-wasm-worker')
           await esbuildWASM.initialize({ wasmModule, worker: true })
           await Deno.mkdir(testDir, { recursive: true })
