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

Platform is not respected in module resolution #3867

Closed
klinki opened this issue Aug 4, 2024 · 3 comments
Closed

Platform is not respected in module resolution #3867

klinki opened this issue Aug 4, 2024 · 3 comments

Comments

@klinki
Copy link

klinki commented Aug 4, 2024

Problem:

I specify the platform to browser, but esbuild doesn't respect that and uses . for module lookup in exports.

Looking for "." in "exports" map in "C:\projects\own\new-proj\consumer\node_modules\ulidx\package.json"

⬥ [VERBOSE] Resolving import "ulidx" in directory "C:\projects\own\new-proj\consumer" of type "import-statement"

Checking for package alias matches
Failed to find any package alias matches
Read 4 entries for directory "C:\projects\own\new-proj\consumer"
No "browser" map found in directory "C:\projects\own\new-proj\consumer"
Searching for "ulidx" in "node_modules" directories starting from "C:\projects\own\new-proj\consumer"
Parsed package name "ulidx" and package subpath "."
Checking for a package in the directory "C:\projects\own\new-proj\consumer\node_modules\ulidx"
Read 4 entries for directory "C:\projects\own\new-proj\consumer"
Read 5 entries for directory "C:\projects\own\new-proj\consumer\node_modules"
Resolved symlink "C:\projects\own\new-proj\consumer\node_modules\ulidx" to "C:\projects\own\new-proj\consumer\node_modules\.pnpm\ulidx@2.4.0\node_modules\ulidx"
The file "C:\projects\own\new-proj\consumer\node_modules\ulidx\package.json" exists
Read 4 entries for directory "C:\projects\own\new-proj\consumer\node_modules\ulidx"
Looking for "." in "exports" map in "C:\projects\own\new-proj\consumer\node_modules\ulidx\package.json"
Using the entry for "."
Checking condition map for one of ["browser", "default", "import", "module"]
The key "import" applies
Checking condition map for one of ["browser", "default", "import", "module"]
The key "types" does not apply
The key "default" applies
Checking path "" against target "./dist/node/index.js"

This results in node module being used instead of browser one and later it will fail with error

✘ [ERROR] Could not resolve "node:crypto"

node_modules/.pnpm/ulidx@2.4.0/node_modules/ulidx/dist/node/index.js:1:19:
  1 │ import crypto from 'node:crypto';
    ╵                    ~~~~~~~~~~~~~

The package "node:crypto" wasn't found on the file system but is built into node. Are you trying
to bundle for node? You can use "--platform=node" to do that, which will remove this error.

How to reproduce:

Just create this simple index.ts file:

import {ulid} from "ulidx";

ulid();

and run:

npm install ulidx@2.4.0
npx esbuild .\index.ts --bundle --platform=browser --log-level=verbose

Expected behavior:
esbuild should read exports:browser instead of exports:.

@evanw
Copy link
Owner

evanw commented Aug 4, 2024

esbuild should read exports:browser instead of exports:.

You are confusing two separate things. In this context, browser is a condition whereas . is a subpath. The subpath . is necessary because you are importing the package itself (import "ulidx" is treated like import "ulidx/." with the way the algorithm works). I assume you want the subpath . to be resolved using the browser condition.

The node_modules/ulidx/package.json looks like this:

{
  "exports": {
    ".": {
      "import": {
        "types": "./dist/index.d.ts",
        "default": "./dist/node/index.js"
      },
      "require": {
        "types": "./dist/index.d.cts",
        "default": "./dist/node/index.cjs"
      },
      "node": {
        "import": {
          "types": "./dist/index.d.ts",
          "default": "./dist/node/index.js"
        },
        "require": {
          "types": "./dist/index.d.cts",
          "default": "./dist/node/index.cjs"
        }
      },
      "browser": {
        "import": {
          "types": "./dist/index.d.ts",
          "default": "./dist/browser/index.js"
        },
        "require": {
          "types": "./dist/index.d.cts",
          "default": "./dist/browser/index.cjs"
        }
      },
      "worker": {
        "import": {
          "types": "./dist/index.d.ts",
          "default": "./dist/browser/index.js"
        },
        "require": {
          "types": "./dist/index.d.cts",
          "default": "./dist/browser/index.cjs"
        }
      }
    }
  },
  ...
}

There is only one exported subpath (called ".") so esbuild is correct for using it. The package provides five ways to resolve the subpath . in order of priority depending on what conditions are currently active: import, require, node, browser, and worker. However, since all path resolutions will either be an import statement or a require call, only the first two path resolutions will ever be used. None of the last three path resolutions (node, browser, and worker) will ever be chosen because they all come after both import and require in priority order.

I'm closing this issue because esbuild seems to be working correctly here. The problem here is that the package definition is broken. It looks like the change that broke it is here: perry-mitchell/ulidx#41. I recommend working with the package author to fix their package. I'm not familiar with how the types condition works, but it's possible their package could be made to work by reverting that change and then adding a default condition that goes last and specifies the default behavior for when no other conditions match. Documentation about how conditions work can be found here: https://nodejs.org/api/packages.html#conditional-exports

@evanw evanw closed this as not planned Won't fix, can't repro, duplicate, stale Aug 4, 2024
@hyrious
Copy link

hyrious commented Aug 4, 2024

I recently just learnt how TypeScript resolves types, so I will put some additional info here.

First of all, in summary of what Evan said, the order of fields in "exports" matters. The first matched condition will take effect (like import > browser in that case).

For TypeScript, the "types" condition in "exports" should always come first. However, in order to make that work, there're 2 other things need to ensure:

@klinki
Copy link
Author

klinki commented Aug 12, 2024

Thanks for info!

I see that someone already reported it in ulidx repository.
I hope it will get resolved soon. Until then I will use custom patch with patch-package.

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

3 participants