Plugin breaks non-JS/TS code

[NfNitLoop]

I'd love to use this to bundle resources that may include Deno code and non-JS code (like CSS/SCSS files).

But adding the plugin to my ESBuild configuration breaks if there are files that aren't TypeScript/JavaScript.

IMO, if this plugin doesn't know how to handle these files, it should just skip them and allow other plugins (or esbuild itself) to handle them.

Reproduction

I've got a minimal reproduction here:

https://github.com/NfNitLoop/deno-loader-bug

With a config like this:

import * as esbuild from "npm:esbuild@0.20.2";
import { denoPlugins } from "jsr:@luca/esbuild-deno-loader@^0.10.3";

const result = await esbuild.build({
  plugins: [
    // ...denoPlugins()
  ],
  entryPoints: [
    "src/mod.ts",
    "src/style.css",
  ],
  outdir: "dist",
  bundle: true,
  format: "esm",
});
esbuild.stop();

This works, but if you uncomment the denoPlugins() line you get errors:

✘ [ERROR] Expected a JavaScript or TypeScript module, but identified a Unknown module. Importing these types of modules is currently not supported.
Specifier: file:///Users/codyc/code/bugs/deno-loader-bug/src/style.css [plugin deno-loader]

[NfNitLoop]

FWIW, I previously rolled my own code that was similar to esbuild-deno-loader, but decided to adopt esbuild-deno-loader to support JSR imports. My loader explicitly only applied to particular kinds of imports (back then, http[s] imports), so it didn't run into this issue. So I expect filtering to just js/ts/jsx/tsx imports here might be a quick fix?

[KyleJune]

The code sited below seems to be the cause of the issue. It assumes that all files can be handled by loadEsm, even files that are not imported. For example, if you add a css file entrypoint to the list with the postcss plugin before or after the deno plugin, it will fail because the deno plugins onLoad function will be called even for css files.

The TODO below should be removed and the onLoad filter should limit it to only being called for files deno would handle, like ts, tsx, js, jsx, and wasm.

https://github.com/lucacasonato/esbuild_deno_loader/blob/main/src/plugin_deno_loader.ts#L360

function onLoad(
  args: esbuild.OnLoadArgs,
): Promise<esbuild.OnLoadResult | null> | undefined {
  if (args.namespace === "file" && isInNodeModules(args.path)) {
    // inside node_modules, just let esbuild do it's thing
    return undefined;
  }
  const specifier = esbuildResolutionToURL(args);
  return loaderImpl.loadEsm(specifier);
}
// TODO(lucacasonato): once https://github.com/evanw/esbuild/pull/2968 is fixed, remove the catch all "file" handler
build.onLoad({ filter: /.*/, namespace: "file" }, onLoad);

[KyleJune]

Another option is changing the onLoad filter to exclude css. That way the default css loader from esbuild or other plugins after it will be used instead. The downside to that is for any other file type, you'd have to still put it before the deno plugin, otherwise the deno plugin's loader will catch it still.

To work around this issue, I plan on just creating an esbuild plugin that will call the default css loader for css files and put it and any css related esbuild plugins before the deno plugin.

[KyleJune]

I was wrong in my last comment about a potential way to workaround the issue. Adding a plugin before it with an onLoad callback won't prevent it from also calling the onLoad callback in the next plugin. The only way to prevent it from getting handled by the next plugin is to mark it as external in an onResolve callback. But esbuild doesn't allow you to set entrypoints as being external, so that won't work either.

I'm going to fork this PR and update it to have more restrictive file type filters for it's onLoad callback that I previously mentioned so that it isn't catching other files that you wouldn't want the deno plugin to handle like css and image files. I'll create a PR to try to upstream it into luca's esbuild_deno_loader but might also publish my fork so that I can use it immediately. I'll add an update here on how to use it for anyone else that encounters this issue. Then I'll yank my fork from jsr once the issue is resolved in this repository.

[lucacasonato]

I have an idea on how to work around this. Let me try it.

[KyleJune]

I have an idea on how to work around this. Let me try it.

Okay, I'll wait for you to try first. I was just going to try myself this weekend but I can wait.

[KyleJune]

If the next release is made today or tomorrow, I'll test it out tonight or tomorrow to verify it is working right, allowing me to build css entrypoints using the postcss esbuild plugin I've been working on.

[NfNitLoop]

I can still reproduce this against v0.11.0-rc.1. But it looks like that's because #142 isn't included in that release.