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

🐛 BUG: Unwanted detection of Node builtins #1475

Closed
isaac-mcfadyen opened this issue Jul 14, 2022 · 5 comments
Closed

🐛 BUG: Unwanted detection of Node builtins #1475

isaac-mcfadyen opened this issue Jul 14, 2022 · 5 comments
Labels
bug Something that isn't working node compat Relating to the differences between the Workers environment and Node

Comments

@isaac-mcfadyen
Copy link
Contributor

isaac-mcfadyen commented Jul 14, 2022

What version of Wrangler are you using?

0.0.0-d35c69f

What operating system are you using?

macOS

Describe the Bug

Wrangler2 attempts to detect the importing of Node built-ins (such as http and fs), seemingly using RegEx.

This is unwanted behavior in many applications. Packages like faunadb are designed for operation in both Node.JS and browser (or Worker) environments. They do this via simple checks like checking if window exists.

Because of the RegEx validation, the build fails even though the adapter would never import those modules in a Worker and so would otherwise work. The node_compat flag can be enabled to work around this, however it adds significant bundle size.

For a good example to reproduce this, try installing and using faunadb. Wrangler's behavior of doing this RegEx check breaks all functionality (and all examples of FaunaDB and Workers).

@isaac-mcfadyen isaac-mcfadyen added the bug Something that isn't working label Jul 14, 2022
@threepointone
Copy link
Contributor

Interesting. We have the check in place because otherwise, esbuild would try to build it, and it would throw an error anyway. I suppose the better behaviour here would be to first check whether the package is available as a regular node package, and if not, just mark it as an external, and downgrade the error to a warning.

@isaac-mcfadyen
Copy link
Contributor Author

I wonder if esbuild would tree-shake those packages? Since they're imported dynamically (import('http'))? Not sure.

Anyways, yeah I'd definitely be good with downgrading that to a warning, so that the user is aware but it doesn't block builds if they know for sure it's fine.

@threepointone
Copy link
Contributor

esbuild won't tree shake dynamic imports.

@penalosa penalosa added the node compat Relating to the differences between the Workers environment and Node label Oct 6, 2022
@caass caass assigned caass and unassigned caass Oct 12, 2022
@Inrixia
Copy link

Inrixia commented Oct 25, 2022

I'm running into this exact issue as well with the only fix being to fork the module and disable the Node.js detection (Inrixia/tweetnacl-js@5a9c25e)...

There REALLY needs to be a way to manually disable throwing on Node.js requires for specific packages or another way to get around this for packages where the developer knows it is a non issue.

@penalosa
Copy link
Contributor

Closing as duplicate of #4050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working node compat Relating to the differences between the Workers environment and Node
Projects
None yet
Development

No branches or pull requests

5 participants