nodejs / readable-stream

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

When upgrading from `4.3.0` to `4.4.0`, using `esbuild --bundle` to build will report an error `Could not resolve "stream"` #516

Closed leizongmin closed 1 year ago

leizongmin commented 1 year ago

This is an example file test.js:

require('readable-stream');

Run the following commands (esbuild@0.17.12):

esbuild --bundle test.js

It outputs:

✘ [ERROR] Could not resolve "stream"

    node_modules/.pnpm/readable-stream@4.4.0/node_modules/readable-stream/lib/stream/promises.js:7:8:
      7 │ require('stream')
        ╵         ~~~~~~~~

  The package "stream" 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.

1 error

If I use version 4.3.0, it will output correctly.

kanongil commented 1 year ago

This seems like another instance of the issue in #513, though this exposes it as a regression!

leizongmin commented 1 year ago

The problem was introduced from this commit https://github.com/nodejs/readable-stream/commit/b345071487c47b3c7b96007b26ea8bbe3ea97d15#diff-241fa22640048b6a147dec92f1ebf253043a1c4c342c7e52402112eb3c58f8fbR7

https://github.com/nodejs/readable-stream/blob/b345071487c47b3c7b96007b26ea8bbe3ea97d15/lib/stream/promises.js#L7

I have another question: why require this module, but not use any properties or methods of stream?

jeswr commented 1 year ago

In order to avoid such regressions in the future can I suggest adding a test to this library which webpacks

import-all-test.js

import * as all from 'readable-stream'

using the config

webpack.config.js

const path = require("path");
module.exports = {
  mode: "development",
  entry: "./integration-tests/import-all-test.js",
  output: {
    path: path.resolve(__dirname, "integration-tests", "dist"),
    filename: "import-all-test.js",
  },
};

The test would just be running npm run test:build where that script is defined as "test:build": "webpack"

mcollina commented 1 year ago

Would you like to send a PR to fix this (adjust the build scripts) or just with the test?

jeswr commented 1 year ago

https://github.com/nodejs/readable-stream/pull/517 should cover the main point - I will see if I can make that config more restrictive after the issue is patched.