sindresorhus / p-limit

Run multiple promise-returning & async functions with limited concurrency
MIT License
1.97k stars 102 forks source link

v5 can break due to subpath imports - node+deno unable to resolve core modules re imports map #75

Closed firxworx closed 3 months ago

firxworx commented 10 months ago

I reported this: https://github.com/nodejs/node/issues/51009 initially reported to tsx here: https://github.com/privatenumber/tsx/issues/425

The minimal repro from the tsx issue (on StackBlitz) uses p-limit for the demo.

I encountered the issue when bumping packages in a TypeScript cli project to current (including bumping p-limit v4 to v5) that runs via tsx (with module "ESNext" in the tsconfig).

The trigger of the issue in p-limit is right here: https://github.com/sindresorhus/p-limit/compare/v4.0.0...v5.0.0#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R2

Given the circumstances I'm not sure if you want to do anything differently, however I figured I'd create an issue here to put it on your radar, in case you want to accommodate on account of the popularity of this package. Thanks!

JonathonRP commented 10 months ago

same issue with deno

firxworx commented 10 months ago

Dang Deno too eh. I will update the issue title to include Deno.

I would throw out there that a vote against using this node feature in p-limit (and perhaps other popular sindresorhus packages) is the discussion here: https://github.com/nodejs/node/issues/49182

The point being: given the lengthy discussion (not to mention the related issue with node + require w/ subpath imports), the nuances of this feature looks to still be up in the air and details for how to implement it are still being fleshed out (and therefore may not be stable either).

JonathonRP commented 9 months ago

Someone else reported a similar issue with esbuild

firxworx commented 9 months ago

@JonathonRP gotcha, thanks for the tip

Note tsx uses esbuild under the hood and is what I was using that led to me encountering + reporting this issue in the first place.

For future note if you comment with the issue link (or link direct to the specific comment), or if you reference the issue id, GitHub will take care of adding the mention to the issue feeds. Its fewer keystrokes than typing out "someone else reported..." and has the benefit of linking things up and tying them together :)

I'm going to comment over on #76 now how this is related

dsherret commented 9 months ago

To be clear, Deno is not affected because of the subpath imports. (Edit: actually I was wrong... looking into it fixing it, but this is a deno bug) An issue is async_hooks is experimental API that Node recommends migrating away from (https://nodejs.org/api/async_hooks.html) and Deno does not properly support all of it (though what p-limit uses may be supported. I'm not sure).

Edit: Opened https://github.com/denoland/deno/pull/21576 and fixed in Deno 1.39.1

segevfiner commented 9 months ago

Seems to also fail in tsup with:

ESM Build failed
Error: Build failed with 1 error:
../../node_modules/.pnpm/p-limit@5.0.0/node_modules/p-limit/index.js:2:28: ERROR: Could not resolve "#async_hooks"
    at failureErrorWithLog (.../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1649:15)
    at .../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1058:25
    at runOnEndCallbacks (.../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1484:45)
    at buildResponseToResult (.../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1056:7)
    at .../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:1085:16
    at responseCallbacks.<computed> (.../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:703:9)
    at handleIncomingPacket (.../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:762:9)
    at Socket.readFromStdout (.../node_modules/.pnpm/esbuild@0.18.20/node_modules/esbuild/lib/main.js:679:7)
    at Socket.emit (node:events:517:28)
    at addChunk (node:internal/streams/readable:368:12)
CJS Build failed
firxworx commented 9 months ago

@segevfiner for sure thanks for the explicit confirmation.

tsup uses esbuild, the same as tsx (which is what I was using when I first hit and reported this issue) so unfortunately this can be expected. You can downgrade p-limit to v 4.x and it should work for you.

narthur commented 7 months ago

I had a similar issue using tsx with p-limit v5. Downgrading to v4 fixed the issue (thanks @firxworx!)

Here's the error I had to help other people searching:

node:internal/url:1393
    throw new ERR_INVALID_URL_SCHEME('file');
          ^

TypeError [ERR_INVALID_URL_SCHEME]: The URL must be of scheme file
    at fileURLToPath (node:internal/url:1393:11)
    at finalizeEsmResolution (node:internal/modules/cjs/loader:1164:20)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1106:16)
    at a._resolveFilename (/Users/work/ProgrammingProjects/[...]/node_modules/.pnpm/tsx@4.7.0/node_modules/tsx/dist/cjs/index.cjs:1:1729)
    at Module._load (node:internal/modules/cjs/loader:985:27)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
    at Queue (/Users/work/ProgrammingProjects/[...]/node_modules/.pnpm/p-limit@5.0.0/node_modules/p-limit/index.js:2:29)
    at Object.<anonymous> (/Users/work/ProgrammingProjects/[...]/node_modules/.pnpm/p-limit@5.0.0/node_modules/p-limit/index.js:4:25)
    at Module._compile (node:internal/modules/cjs/loader:1376:14) {
  code: 'ERR_INVALID_URL_SCHEME'
}

Node.js v20.11.0
segevfiner commented 4 months ago

Seems like it is an issue in Node.js itself: https://github.com/nodejs/node/issues/49257

crypt0miester commented 3 months ago

hello, using patch-package you can do the following:

diff --git a/node_modules/p-limit/index.js b/node_modules/p-limit/index.js
-import {AsyncResource} from '#async_hooks';
+import {AsyncResource} from './async-hooks-stub';

hope that helps someone until the issue is resolved.

fer-ri commented 3 months ago

hello, using patch-package you can do the following:

diff --git a/node_modules/p-limit/index.js b/node_modules/p-limit/index.js
-import {AsyncResource} from '#async_hooks';
+import {AsyncResource} from './async-hooks-stub';

hope that helps someone until the issue is resolved.

Thanks, it helped me.

For clarity, I manually edited the node_modules/p-limit/index.js file and changed this part to import {AsyncResource} from './async-hooks-stub';

sindresorhus commented 3 months ago

The latest version no longer uses subpath exports: https://github.com/sindresorhus/p-limit/releases/tag/v6.0.0