[@rollup/plugin-node-resolve]: Should not consider node modules with trailing slash as built-in.

[philfontaine]

• Rollup Plugin Name: @rollup/plugin-node-resolve
• Rollup Plugin Version: 13.3.0
• Rollup Version: 2.75.6
• Operating System (or Browser): Windows 11
• Node Version: 16.15.0
• Link to reproduction (⚠️ read below): https://github.com/philfontaine/bug-rollup-string-encoder

Expected Behavior

node modules with trailing slash should not be resolved as built-in, as explained in this comment https://github.com/nodejs/readable-stream/issues/391#issuecomment-446520961

I can't find the supporting docs to confirm that's how modules with forward slash should be resolved, but it seems to be this way since it breaks when it's required from the rollup bundle.

In my use case, I have a bunch of libs that depend on readable-stream@3 which contains require('string_decoder/').

Actual Behavior

The plugin thinks it's built-in because the is-builtin-module module returns true, so it does not resolve it.

Additional Information

To reproduce on the repo:

1. yarn build
2. node dist/index.js

To fix:

Seems like it just needs an endsWith('/') check on the module name to confirm that it's not built-in.

This maybe should be fixed upstream on the is-builtin-module package?

[ericmorand]

Thanks for opening the issue, I was about to.

In case someone is looking for a workaround, I had success with using rollup/plugin-replace the following way:

replacePlugin({
      values: {
        "'string_decoder/'": "'string_decoder/lib/string_decoder.js'"
      },
      delimiters: ['', '']
    })

[drewsynan]

I also ran into this issue trying to bundle readable-stream as a transitive dep. I wan't able to get @ericmorand's solution to work, but this worked for me:

import { nodeResolve } from '@rollup/plugin-node-resolve';
import isBuiltin from 'is-builtin-module';

...

plugins = [
   nodeResolve({
      resolveOnly: (module) => module === 'string_decoder' || !isBuiltin(module),
      preferBuiltins: false,
      exportConditions: ['node'],
   }),
   ...
]
...

ymmv since this seems super hacky and like it would mess with libs that are expecting the actual builtin, but in my case it worked!

[stale[bot]]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ

[guybedford]

This is a standard CommonJS compat requirement, keeping it open for now.

[rsweeneydev]

I've submitted a pull request to is-buildin-module to fix this issue.

https://github.com/sindresorhus/is-builtin-module/pull/13

I tested the fix locally and it resolved this issue for me in rollup.

[stale[bot]]

Hey folks. This issue hasn't received any traction for 60 days, so we're going to close this for housekeeping. If this is still an ongoing issue, please do consider contributing a Pull Request to resolve it. Further discussion is always welcome even with the issue closed. If anything actionable is posted in the comments, we'll consider reopening it. ⓘ