node-fetch/fetch-blob

Module not found: Error: Can't resolve 'stream/web'

ctq123 opened this issue ยท 20 comments

ctq123 commented

Reproduction

Module not found: Error: Can't resolve 'stream/web' in '/Users/alan/project/duapp/vscode/packages/du-i18n/node_modules/fetch-blob'

image

Steps to reproduce the behavior:

  1. yarn
  2. yarn start
  3. start fail

Expected behavior

start success

Screenshots

Your Environment
vscode extension development

software version
node-fetch 3.0.0
node v14.17.3
npm 6.14.13
Operating System macos

Additional context

jimmywarting commented

don't get why u recive a error. it's wrapped around a try/catch

LinusU commented

@ctq123 I believe that this is only a warning right? When WebPack detects an import inside try/catch it just prints a warning, the bundle should still be okay to use...

btakita commented

Running sapper, rollup, on AWS Lambda with version nodejs14.x.

I'm getting the error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'stream' imported from /path/to/apps/web/server2.js
    at packageResolve (internal/modules/esm/resolve.js:664:9)
    at moduleResolve (internal/modules/esm/resolve.js:705:18)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:819:11)
    at Loader.resolve (internal/modules/esm/loader.js:89:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:242:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:73:40)
    at link (internal/modules/esm/module_job.js:72:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Rollup treats 'stream/web' as an external & hoists the require to the top of the js payload file.

The output looks something like this:

import require$$0 from 'stream/web';
// ...
if (!globalThis.ReadableStream) {
  try {
    Object.assign(globalThis, require$$0);
  } catch (error) {
		// TODO: Remove when only supporting node >= 16.5.0
    Object.assign(globalThis, ponyfill_es2018.exports);
  }
}
  • ๐Ÿ‘3
jimmywarting commented

Hmm, i did not account for bundlers to hoist stuff like this. This kind of conditional import should really be converted to top level await + async import to match the same behavior...
Should report/suggest it to rollup

wood1986 commented

webpack should add stream/web to NodeTargetPlugin. See my PR

ctq123 commented

bundle should still be okay to use.

yes๏ผŒit don't affect packaging, but it's not friendly to runtime

dominikg commented

this issue prevents sveltekit from updating to node-fetch 3.0.0

I've tried looking into it yesterday and wonder about a couple of things:

  1. you are not tagging node internal modules with node: which is recommended for esm.
    Would tagging the internal modules make life easier for bundlers to detect new/unknown node internals?

  2. the package.json files of node-fetch and fetch-blob do not contain "exports" mapping or "module" fields
    they may not be strictly required, but exporting package.json is a common courtesy and using exports is recommended here too sindresorhus/meta#15

  3. fetch-blob uses require in streams.cjs. (to avoid using top level await for node12 support i guess)
    Are there ways to avoid this direct require?

  • ๐Ÿ‘1
jimmywarting commented
  1. this old issue made me belive i targeted 12.17 #98 but the node.engine is set to 12.20. so we should be okey to use node:
  2. so far i have not had any problems with it with the way it's right now.
  3. yes it's to avoid top-level await for 12.20 support... haven't come up with any better solution yet

and lazily dynamic import

dynamic import is challenging const stream = new Blob().stream() is all sync would be different if stream returned a promise...

i have tried to best come up with a solution that don't import/depend on core node stuff. so that require can gracefully fail in Deno and Browser
like this result: https://cdn.jsdelivr.net/npm/fetch-blob/+esm
but others produce stuff like: https://jspm.dev/fetch-blob that fails totally

benmccann commented

Here's the problematic line for reference:

fetch-blob/streams.cjs

Line 7 in 46476fa

Object.assign(globalThis, require('stream/web'))

jimmywarting commented

Would anyone be able to report this as a bug to Rollup that it's hoisting the import outside of the try / catch?

Kind of already did: rollup/rollup#3621

jimmywarting commented

I'm wondering if anyone has ideas for reproducing this for a bug report?

It could also be someones else fault... I mean the file is in a .cjs extension. (cuz it's suppose to stay in cjs format) but for some reason it's being rewritten into import require$$0 from 'stream/web';

benmccann commented

This looks like it could be related: https://github.com/rollup/plugins/tree/master/packages/commonjs#ignoretrycatch. Vite includes @rollup/plugin-commonjs by default. Though I tried setting it to true and it didn't seem to change anything

  • ๐Ÿ‘1
benmccann commented

Actually, that option did fix the error for me. I just set it in the wrong place the first time around. Here's a working version: sveltejs/kit#2422

I think the default value for this option should be changed in @rollup/plugin-commonjs to avoid this issue: rollup/plugins#1004

jimmywarting commented

Thank you @benmccann for finding the root cause and reporting issues

benmccann commented

@rollup/plugin-commonjs 21.0.0 is out now, so hopefully this should be fixed. I've sent PRs to upgrade SvelteKit sveltejs/kit#2539 and Vite vitejs/vite#5173

  • ๐Ÿ‘1
  • ๐Ÿš€1
benmccann commented

Vite 2.6.3 and SvelteKit 1.0.0-next.180 have been released and use the fixed version of @rollup/plugin-commonjs

jimmywarting commented

good to hear, dose it work okey now to include fetch-blob?

benmccann commented

Yes

  • ๐ŸŽ‰1