Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Rollup plugin with support for Node built-in modules #288

Closed
gaui opened this issue Dec 24, 2018 · 7 comments
Closed

Add Rollup plugin with support for Node built-in modules #288

gaui opened this issue Dec 24, 2018 · 7 comments

Comments

@gaui
Copy link

gaui commented Dec 24, 2018

When importing child_process and targeting node, I still get this error from #238 :

'child_process' is imported by src\command.ts, but could not be resolved – treating it as an external dependency
Error: Could not load child_process (imported by src/command.ts): ENOENT: no such file or directory, open 'child_process'

This can be bypassed by adding --external child_process argument. Still I get this message:

Creating a browser bundle that depends on Node.js built-in modules ('path', 'assert', 'util' and 'events'). You might need to include https://www.npmjs.com/package/rollup-plugin-node-builtins

So please add package rollup-plugin-node-builtins so Rollup knows how to detect Node built-in modules and resolve them appropriately.

@marvinhagemeister
Copy link
Collaborator

Good catch, can you make a PR?

@lukeed
Copy link
Contributor

lukeed commented Dec 24, 2018

I think we can skip the plugin & do a special case for --external natives that adds them:

require('module').builtinModules || Object.keys(process.binding('natives'))

@gaui
Copy link
Author

gaui commented Dec 25, 2018

@lukeed Spot on. But wouldn't that resolution logic belong in Rollup?

@lukeed
Copy link
Contributor

lukeed commented Dec 25, 2018

@gaui Personally, I would vote yes, but realistically it's not something Rollup can/wants to do because you may have shims/custom modules that share the same names, and the above would likely (have to) override anything non-native with the native version.

@gaui
Copy link
Author

gaui commented Dec 25, 2018

If you're naming your modules the same as the natives, you're doing something wrong. 😄

@gaui
Copy link
Author

gaui commented Dec 25, 2018

@Rich-Harris what do you think?

@gaui
Copy link
Author

gaui commented Dec 27, 2018

@lukeed Is it possible to mark those modules returned from the above function, as external so Rollup knows? Not familiar with microbundle internals.

developit added a commit that referenced this issue Apr 18, 2020
This option was enabled by default at some point in the node-resolve plugin, which should already have fixed #303 and #288. It still prints a warning though, which is illogical when we're compiling for a Node target. This silences that warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants