nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 227 forks source link

process is undefined in lib/internal/streams/pipeline.js #496

Closed rybesh closed 1 year ago

rybesh commented 1 year ago

I am using N3.js which recently upgraded readable-stream from 3.6.0 to 4.0.0. Now I can no longer build my project using browserify; I get the following error:

node_modules/.bin/browserify -d -o periodo-client.js modules/periodo-app/src/index.js
Error: Can't walk dependency graph: ENOENT: no such file or directory, lstat '/home/runner/work/periodo-tests/periodo-tests/periodo-client/process'
    required by /home/runner/work/periodo-tests/periodo-tests/periodo-client/node_modules/n3/node_modules/readable-stream/lib/internal/streams/pipeline.js
mcollina commented 1 year ago

Take a look at https://github.com/nodejs/readable-stream#usage-in-browsers

rybesh commented 1 year ago

I'm not using Webpack 5, so I don't understand how that passage applies to my case.

mcollina commented 1 year ago

cc @ShogunPanda

vweevers commented 1 year ago

There are several issues here:

  1. Taking the Webpack 5 approach, instead of requiring bundlers to inject shims for node core modules (https://github.com/nodejs/readable-stream/issues/439#issuecomment-634888208). Meaning we now do require('process') instead of using a global: https://github.com/nodejs/readable-stream/blob/7fdb3b4ec5135e586d5c6695f7e08d04f268b1cb/lib/internal/streams/pipeline.js#L2
  2. This browserify bug that breaks require('process'): https://github.com/browserify/browserify/issues/1986
  3. This workaround that assumes the process package always lives at ./node_modules/process (i.e. that it's not hoisted): https://github.com/nodejs/readable-stream/blob/7fdb3b4ec5135e586d5c6695f7e08d04f268b1cb/process#L3
  4. The files declaration in package.json that's missing the process file: https://github.com/nodejs/readable-stream/blob/7fdb3b4ec5135e586d5c6695f7e08d04f268b1cb/package.json#L26-L30
vweevers commented 1 year ago

@ShogunPanda When you worked on this, did you try require('process/') instead of require('process')? If that works (for all bundlers) then we don't need the workaround.

bergos commented 1 year ago

It looks like the workaround with require('process/') would work on Node.js and all bundlers.

I didn't use the workaround because I wanted to keep the Node.js behavior to directly load the internal process module. But it looks like the process-shim doesn't contain much logic, so I guess it would be ok to have this one additional require.

vweevers commented 1 year ago

@bergos Ah, I see now that it's your code (https://github.com/nodejs/readable-stream/pull/489) rather than @ShogunPanda's. Sorry, I missed that PR. Side note: if bundler configuration is no longer required, then Usage In Browsers is outdated.

I'm not a fan of the general approach, but in the interest of moving forward, I vote for require('process/') too.

mcollina commented 1 year ago

I'm ok in adding the process-shim.