evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
38.21k stars 1.15k forks source link

Incorrect warning regarding "default" conditional export #3887

Closed tvsbrent closed 2 months ago

tvsbrent commented 3 months ago

With the change introduced for this issue, #3867, we are now seeing the following error from esbuild:

▲ [WARNING] The condition "default" here will never be used as it comes after both "import" and "require" [package.json]

The package.json in this case has these conditional exports:

  "exports": {
    ".": {
      "node": "./dist/index.js",
      "require": "./dist/index.js",
      "import": "./dist/index.esm.js",
      "default": "./dist/index.esm.js"
    },
    "./dist/index.css": {
      "default": "./dist/index.css"
    }
  },

According to the NodeJS documentation, it appears default should always be last, so this warning feels incorrect?

hyrious commented 3 months ago

The Node.js documentation also said:

Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order.

The matching algorithm should test conditions from top to bottom, until some case matches. Since there's only 2 importing style in JavaScript (require or import), they must at least much one. Thus the default condition will never be used if present after import and require. So the warning is correct here to inform package authors to write correct package.json.

tvsbrent commented 3 months ago

Based on reviewing the code in that change further and your comment @hyrious , I am going to close this. I'm worried there may be some weird tool in our toolchain that doesn't work as one would hope, and that default is needed in that case, but I don't have a specific situation right now.

evanw commented 3 months ago

I'm reopening this as I agree that this warning shouldn't trigger for default like this in my opinion. I missed this case due to an oversight. I'm not sure how it could come up in practice (maybe some tool doesn't support import or require?) but the ability to have a catch-all default clause without a warning seems reasonable to me. Sorry about the warning noise in the meantime. You can hide the warning with --log-override:package.json=silent for now if you'd like.

bhousel commented 2 months ago

I'm also seeing this warning for several of my projects that are set up like this:

  "exports": {
    "import": "./thing.mjs",
    "require": "./thing.cjs",
    "*": "./*"
  },

I've used this * export in cases where the project exports both code and data - where consumers might need to do import stuff from 'mypackage/data/stuff.json';

(this might not be best practice though)

evanw commented 2 months ago

I'm also seeing this warning for several of my projects that are set up like this:

  "exports": {
    "import": "./thing.mjs",
    "require": "./thing.cjs",
    "*": "./*"
  },

I've used this * export in cases where the project exports both code and data - where consumers might need to do import stuff from 'mypackage/data/stuff.json';

(this might not be best practice though)

Sorry, I'm confused. How would that work? In the example you gave, that import is a module instantiation error. Running that example in node gives the following error:

node:internal/modules/esm/resolve:303
  return new ERR_PACKAGE_PATH_NOT_EXPORTED(
         ^

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './data/stuff.json' is not defined by "exports" in node_modules/mypackage/package.json imported from entry.mjs
    at exportsNotFound (node:internal/modules/esm/resolve:303:10)
    at packageExportsResolve (node:internal/modules/esm/resolve:650:9)
    at packageResolve (node:internal/modules/esm/resolve:828:14)
    at moduleResolve (node:internal/modules/esm/resolve:918:18)
    at defaultResolve (node:internal/modules/esm/resolve:1148:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:528:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:497:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:231:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:97:39)
    at link (node:internal/modules/esm/module_job:96:36) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}

Node.js v22.0.0