[BUG] `external` doesn't always work in snowpack ^3.1

akdor1154 commented 3 years ago

Bug Report Quick Checklist

[x] I am on the latest version of Snowpack & all plugins.
[x] I use package manager npm (Fill in: npm, yarn, pnpm, etc).
[x] I run Snowpack on OS Linux (Fill in: Windows, Mac, Linux, etc).
[x] I run Snowpack on Node.js v12+ (14.16.0)

Describe the bug

Even though I set external: ['fs'], I still get an error when running snowpack dev, and I can't build my project unless I set polyfillNode: true. This only appears in 3.1 versions, 3.0 seems to work ok.

To Reproduce

We can't fix bugs that we can't see for ourselves. Issues often need to be closed if this section is skipped.

npm init snowpack-app --template @snowpack/app-template-blank
npm add -D snowpack@3.1.2
npm add antlr4@4.9.2

cat <<EOF > public/index.html
<!doctype html>
<html>
<head>
    <script type="module">
        import antlr4 from 'antlr4';
    </script>
</head>
<body>Hi</body>
</html>
EOF

cat <<EOF > snowpack.config.js
/** @type {import("snowpack").SnowpackUserConfig } */
module.exports = {
mount: {
public: { url: '/', static: true },
src: { url: '/dist' },
},
packageOptions: {
external: ['fs']
}
};
EOF

npx snowpack dev
See error! Error can be worked around by adding polyfillNode: true but this isn't really a solution.

Expected behavior

No error

IgnusG commented 3 years ago

There is some weird behavior going on since the changes in 3.1.0. I'm having similar issues with react-cosmos: https://github.com/react-cosmos/react-cosmos/issues/1320

Nivisnum commented 3 years ago

I've tried to look into this (I really want to make it work with react-cosmos) but sadly this seems like correct behavior - we are telling snowpack to just not touch the import when transpiling the sources but it would be natural to expect that we will provide this dependency on runtime (I've recreated the issue and the error happens when we actually request the dependency from the dev server).

Let's look at skypack for the antlr4 and similar dependency that I'm fighting with: https://cdn.skypack.dev/-/antlr4@v4.9.2-JifkgbNObkYOM5jksEE6/dist=es2020,mode=imports/optimized/antlr4.js https://cdn.skypack.dev/-/xmlhttprequest-ssl@v1.6.3-ZnvqsXZiURMNnfCeQd23/dist=es2020,mode=imports/optimized/xmlhttprequest-ssl.js

As you can see we are importing something weird:

import "/error/node:fs?from=antlr4";

The comment explains quite a lot:

/*
 * [Package Error] [Package Error] "fs" does not exist. (Imported by "antlr4").
 *
 * The package "antlr4" depends on this Node.js built-in module.
 * Node.js built-in modules (like "fs", "http", etc.) are Node.js-specific, and do
 * not exist in non-Node environments like Deno or the web browser. Skypack CDN polyfills
 * most of these modules for you automatically, but this one could not be polyfilled.
 *
 * How to fix:
 *   - Let the package author know that you'd like to run their package in the browser.
 *   - Use https://skypack.dev/ to find a web-friendly alternative to find another package.
 */

I've checked by trying the streaming imports and importing it directly as es module. I assume that snowpack should have similar to skypack when using local option.

So it seems that polyfillNode: true is actually the correct solution (or dependency maintainer could separate web/node code and provide additional info in exports in package.json). If someone smarter than me could confirm (or deny) this I would be happy :)

FredKSchott / snowpack