FredKSchott / snowpack

ESM-powered frontend build tool. Instant, lightweight, unbundled development. ✌️
https://www.snowpack.dev
MIT License
19.48k stars 922 forks source link

[BUG] Prepare fails when any file depends on Node modules, even with `polyfillNode: true` #3045

Open fwouts opened 3 years ago

fwouts commented 3 years ago

Bug Report Quick Checklist

Describe the bug

Snowpack's preparation step (which generates node_modules/.cache) crashes when any file depends on a Node module such as fs. This happens even when setting polyfillNode: true.

yarn snowpack dev
yarn run v1.22.10
$ /Users/fwouts/dev/react-snowpack/node_modules/.bin/snowpack dev
[09:40:25] [snowpack] Welcome to Snowpack! Because this is your first time running
this project, Snowpack needs to prepare your dependencies. This is a one-time step
and the results will be cached for the lifetime of your project. Please wait...
[09:40:26] [snowpack] Package "fs" not found. Have you installed it?
error Command failed with exit code 1.

To Reproduce

Clone https://github.com/fwouts/react-snowpack/tree/bug-fs and run yarn start.

The commit that introduces the issue is https://github.com/fwouts/react-snowpack/commit/7775686328cf6cbefa782160c23a3a8d2661e34f.

The issue only appears when there isn't already a node_modules/.cache directory.

Expected behavior

Snowpack should correctly identify that the imported package comes from Node and shouldn't complain when it's supposed to be polyfilled.

Anything else?

Setting external: ["fs"] does seem to do the job.

fwouts commented 3 years ago

I should add that, when I experience this issue in practice, it's more often that another file which won't be used in the context of Snowpack (e.g. a webpack config or a server-side helper) depends on Node.

In that context, I found a neat workaround: simply write echo -n "v1" > .node_modules/.cache/snowpack/development/.meta. This makes it possible to skip Snowpack's initial preparation step, and only prepare node modules that are actually required on demand.

FredKSchott commented 3 years ago

This is a great callout that I've been meaning to address for a while now, appreciate you starting the conversation.

The idea is that Snowpack apps are meant to be served to the browser, so import 'fs' shouldn't work. I had thought that we were polyfilling this with polyfillNode, but that may only affect internal npm package imports (so that if one of your dependencies used fs, it wouldn't break).

I think polyfillNode could be better served by some sort of nodeBuiltins: 'allow' | 'polyfill' | 'disable' config. We still don't want users importing Node.js builtins in their frontend apps if we can help it.

fwouts commented 3 years ago

Thanks @FredKSchott. Yeah, that makes sense.

What's problematic is that when you try to use Snowpack in a codebase that happens to also contains some server code (or simply a webpack config), it will try to prepare all the packages they depend on too, and in this case fail.

Looking at the codebase, I suspect it's because Snowpack defaults to scanning all files within the directory (see source).

I wonder if that behaviour could perhaps be disabled? In my case, I'd prefer to set knownEntryPoints in the config and ensure that it only scans them and their dependencies.