Panic when using subpath imports for bundled browser build

[znewsham]

I'm trying to use node's subpath imports syntax as follows to get around the issue of absolute imports not being relative to the project root (the reason for using the subpath isn't super relevant). Somewhat confusingly the path I'm trying to import is /imports - which I use #imports/* to represent. Note - I've already checked #2913 - I don't think it's relevant as I'm already on 0.17.17

{
  ...
  imports: {
    "#imports/*": "./imports/*"
  }
  ...
}

This works just fine when doing a server build - but not when doing a client build. Both use the bundle option (however the server build does do some very custom queueing work). On the client build I get the following error:

✘ [ERROR] panic: runtime error: invalid memory address or nil pointer dereference (while parsing "client/main.ts")

  debug.Stack (runtime/debug/stack.go:24)
  helpers.PrettyPrintedStack (internal/helpers/stack.go:9)
  bundler.parseFile.func1 (internal/bundler/bundler.go:189)
  panic (runtime/panic.go:884)
  resolver.resolverQuery.finalizeImportsExportsResult (internal/resolver/resolver.go:2420)
  resolver.resolverQuery.loadPackageImports (internal/resolver/resolver.go:2032)
  resolver.resolverQuery.loadNodeModules (internal/resolver/resolver.go:2113)
  resolver.resolverQuery.resolveWithoutRemapping (internal/resolver/resolver.go:896)
  resolver.resolverQuery.resolveWithoutSymlinks (internal/resolver/resolver.go:883)
  resolver.(*Resolver).Resolve (internal/resolver/resolver.go:512)
  bundler.RunOnResolvePlugins (internal/bundler/bundler.go:871)
  bundler.parseFile (internal/bundler/bundler.go:393)
  bundler.(*scanner).maybeParseFile (internal/bundler/bundler.go:1368)

The triggering import (in client/main.ts) looks like this:

import "#imports/startup"

Interestingly, if I change the import to #imports/startup/index.ts - it works - Note: index.ts is required - #imports/startup/index fails with a different error.

The arguments I'm passing to esbuild are as follows (abridged)

  const context = await esbuild.context({
    absWorkingDir: process.cwd(), 
    minify: isProduction,
    entryPoints: [entryPoint],
    outdir,
    preserveSymlinks: true,
    conditions: [isProduction ? 'production' : 'development', archName],
    external: [
      '*.jpg',
      '*.png',
      '*.svg',
      '/fonts/*',
    ],
    sourcemap: 'linked',
    logLevel: 'error',
    define: {
      ...
    },
    plugins: [
      addJsExtension(buildRoot), // potentially relevant - but explicitly ignores those imports that start with . / or #
      onStart,
      onEnd
    ],
    splitting: true, // same behaviour false too
    bundle: true,
    format: archName === 'web.browser' ? 'esm' : 'iife',
    metafile: true,
  });

[evanw]

Please provide a self-contained way to reproduce the issue.

[znewsham]

https://github.com/znewsham/test-esbuild

node esbuild.js

[evanw]

Thanks for the report. This crash happens as esbuild is attempting to generate a helpful error message for this bad subpath import. The error message should be:

✘ [ERROR] Could not resolve "#imports/startup"

    test.ts:1:7:
      1 │ import '#imports/startup';
        ╵        ~~~~~~~~~~~~~~~~~~

  Importing the directory "./imports/startup" is forbidden by this package:

    package.json:11:18:
      11 │     "#imports/*": "./imports/*"
         ╵                   ~~~~~~~~~~~~~

  The presence of "imports" here makes importing a directory forbidden:

    package.json:10:2:
      10 │   "imports": {
         ╵   ~~~~~~~~~

  Import from "/index.ts" to get the file "imports/startup/index.ts":

    test.ts:1:24:
      1 │ import '#imports/startup';
        │                         ^
        ╵                         /index.ts

  You can mark the path "#imports/startup" as external to exclude it from the bundle, which will
  remove this error.

This crash will be fixed in the next version of esbuild. In the meantime, the workaround is to fix the incorrect import path.

[znewsham]

Ah, that's unfortunate - I guess esbuild isn't compatible with this form of resolution?

This doesn't seem to match up with esbuild's behaviour of relative paths. For example, if I were to import ./imports/startup - esbuild would happily resolve to the TS file - the code in question is loaded by both the client and server, on the server I can't use .ts and must either omit the extension entirely or use .js. I guess the workaround here would be to use a custom plugin to provide a resolve function for this case?

[znewsham]

As I re-read your comment - I think I understand, on the server I use an experimental flag to support this (and in future versions of node will need to use a custom loader)

[evanw]

The problem is that the behavior of the imports field is specified by node, not esbuild, and the specification does not permit the loading of a directory. You'll also get that if you try this in node itself (albeit with a slightly different error message):

$ mv test.ts test.js
$ node test.js 
node:internal/errors:491
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_UNSUPPORTED_DIR_IMPORT]: Directory import './imports/startup' is not supported resolving ES modules imported from ./test.js
    at new NodeError (node:internal/errors:400:5)
    at finalizeResolution (node:internal/modules/esm/resolve:319:17)
    at moduleResolve (node:internal/modules/esm/resolve:945:10)
    at defaultResolve (node:internal/modules/esm/resolve:1153:11)
    at nextResolve (node:internal/modules/esm/loader:163:28)
    at ESMLoader.resolve (node:internal/modules/esm/loader:842:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:77:40)
    at link (node:internal/modules/esm/module_job:76:36) {
  code: 'ERR_UNSUPPORTED_DIR_IMPORT',
  url: 'file://./imports/startup'
}

[znewsham]

Oh very interesting - I guess I never got this far. It seems there is no way around this - the problem I was trying to solve is converting a legacy codebase and I didn't want to have to convert all my /imports paths to the relevant ../(../)+ paths. Interestingly, esbuild bundling for the client doesn't complain about the absolute paths, but node does without the custom loader.

Maybe I just bite the bullet and convert the damn things. Thanks for looking!