Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should it fallback to main fields when no condition matches? #1956

Closed
ruanyl opened this issue Jan 24, 2022 · 4 comments
Closed

Should it fallback to main fields when no condition matches? #1956

ruanyl opened this issue Jan 24, 2022 · 4 comments

Comments

@ruanyl
Copy link

ruanyl commented Jan 24, 2022

Take tslib as example:

require("esbuild").buildSync({
  entryPoints: ["tslib"],
  bundle: true,
  outfile: "./dist/tslib.js",
  format: "esm",
  target: "esnext",
});

tslib has

    "main": "tslib.js",
    "module": "tslib.es6.js",
    "jsnext:main": "tslib.es6.js",
    "exports": {
        ".": {
            "module": "./tslib.es6.js",
            "import": "./modules/index.js",
            "default": "./tslib.js"
        },
        "./": "./"
    }

Esbuild bundles ./tslib.js with above setting while I was expecting it to use ./tslib.es6.js as I set format: "esm", target: "esnext"

In the case that uses tslib itself as entry point, should esbuild use "main fields"(browser, module, main) instead of reading exports which will then fallback to exports.default?

@ruanyl
Copy link
Author

ruanyl commented Jan 24, 2022

Another example is is-plain-object, it has

  "main": "dist/is-plain-object.js",
  "module": "dist/is-plain-object.mjs",
  "exports": {
    ".": {
      "import": "./dist/is-plain-object.mjs",
      "require": "./dist/is-plain-object.js"
    },
    "./package.json": "./package.json"
  },

with esbuild config:

require("esbuild").buildSync({
  entryPoints: ["is-plain-object"],
  bundle: true,
  outfile: "./dist/is-plain-object.js",
  format: "esm",
  target: "esnext",
  platform: "browser",
});

Getting error:

✘ [ERROR] Could not resolve "is-plain-object"

  The path "." is not currently exported by package "is-plain-object":

    node_modules/is-plain-object/package.json:26:13:
      26 │   "exports": {
         ╵              ^

  None of the conditions provided ("import", "require") match any of the currently active conditions
  ("browser", "default"):

    node_modules/is-plain-object/package.json:27:9:
      27 │     ".": {
         ╵          ^

/Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:2148
      throw reject;
      ^

Error: Build failed with 1 error:
error: Could not resolve "is-plain-object"
    at failureErrorWithLog (/Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:1557:15)
    at /Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:1215:28
    at runOnEndCallbacks (/Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:1133:65)
    at buildResponseToResult (/Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:1213:7)
    at /Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:1322:14
    at /Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:637:9
    at handleIncomingPacket (/Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:734:9)
    at Socket.readFromStdout (/Users/yulongruan/project/esbuild/node_modules/esbuild/lib/main.js:604:7)
    at Socket.emit (node:events:390:28)
    at addChunk (node:internal/streams/readable:315:12) {
  errors: [
    {
      detail: undefined,
      location: null,
      notes: [
        {
          location: {
            column: 13,
            file: 'node_modules/is-plain-object/package.json',
            length: 1,
            line: 26,
            lineText: '  "exports": {',
            namespace: '',
            suggestion: ''
          },
          text: 'The path "." is not currently exported by package "is-plain-object":'
        },
        {
          location: {
            column: 9,
            file: 'node_modules/is-plain-object/package.json',
            length: 1,
            line: 27,
            lineText: '    ".": {',
            namespace: '',
            suggestion: ''
          },
          text: 'None of the conditions provided ("import", "require") match any of the currently active conditions ("browser", "default"):'
        }
      ],
      pluginName: '',
      text: 'Could not resolve "is-plain-object"'
    }
  ],
  warnings: []
}

@evanw
Copy link
Owner

evanw commented Jan 25, 2022

For the first one: esbuild appears to be working correctly. Nothing about format affects path resolution because format sets the output format, not the input format. You could add conditions: ['module'] if you want tslib.es6.js to be picked instead.

For the second one: yes, perhaps esbuild should fall back to the legacy module/main logic in this case (specifically when the path resolution is neither from an import nor from a require). Entry points are a strange edge case here that doesn't really come up in node itself, which is where the specification is designed, so it might be ambiguous what should happen. The only other edge case I can think of where this could come up is path resolution of a path in a CSS file.

The exports field is documented here: https://nodejs.org/api/packages.html#conditional-exports. I notice the documentation says that require and import are always mutually exclusive each other, which is interesting. Node's specification doesn't have to handle esbuild's cases such as entry points and CSS imports. But it could mean that another potential approach is for esbuild to somehow pick either require or import to apply in these edge cases since apparently exactly one of them is always supposed to be specified. I wonder what the people who designed this specification would think esbuild should do here.

@ruanyl
Copy link
Author

ruanyl commented Jan 25, 2022

Thank you @evanw for the detailed explanation! And right, adding conditions: ['module'] is what I'm doing now for 1st case. Looking forward to see the updates for the 2nd case :)

@evanw
Copy link
Owner

evanw commented Jan 25, 2022

I asked for clarification on the spec, and there's a pretty long thread about it here: nodejs/node#41686. It sounds like there isn't strong consensus for what to do about this from node's side, since this isn't a case that exists in node. And both Webpack and Rollup appear to always treat this case as an import so esbuild probably should too. I'll change esbuild to do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants