workers-sdk icon indicating copy to clipboard operation
workers-sdk copied to clipboard

🐛 BUG: Unwanted detection of Node builtins

Open isaac-mcfadyen opened this issue 3 years ago • 3 comments
trafficstars

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 avatar Jul 14 '22 12:07 isaac-mcfadyen

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.

threepointone avatar Jul 14 '22 15:07 threepointone

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.

isaac-mcfadyen avatar Jul 14 '22 17:07 isaac-mcfadyen

esbuild won't tree shake dynamic imports.

threepointone avatar Jul 15 '22 09:07 threepointone

I'm running into this exact issue as well with the only fix being to fork the module and disable the Node.js detection (https://github.com/Inrixia/tweetnacl-js/commit/5a9c25e0045dadcb2be68705f5e841fa2db3715b)...

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.

Inrixia avatar Oct 25 '22 07:10 Inrixia

Closing as duplicate of #4050

penalosa avatar Sep 29 '23 14:09 penalosa