Modules marked as external should always be treated as external (#3408

) * Modules marked as `external` should always be treated as external * Renamed external variable for clarity
FredKSchott · Jun 4, 2021 · aed6a8f · aed6a8f · vercel · Jun 4, 2021
1 parent cd6ec78
commit aed6a8f
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 13 deletions.
diff --git a/snowpack/src/sources/local.ts b/snowpack/src/sources/local.ts
@@ -43,7 +43,7 @@ directory and Snowpack will recreate it on next run.
 
 [.meta.version=2]`;
 
-const NEVER_PEER_PACKAGES: string[] = [
+const NEVER_PEER_PACKAGES: Set<string> = new Set([
   '@babel/runtime',
   '@babel/runtime-corejs3',
   'babel-runtime',
@@ -53,7 +53,7 @@ const NEVER_PEER_PACKAGES: string[] = [
   'whatwg-fetch',
   'tslib',
   '@ant-design/icons-svg',
-];
+]);
 
 function isPackageCJS(manifest: any): boolean {
   return (
@@ -510,17 +510,24 @@ export class PackageSourceLocal implements PackageSource {
           .filter((spec) => spec.startsWith(packageUID))
           .map((spec) => spec.substr(packageUID.length + 1));
         // TODO: external should be a function in esinstall
-        const externalPackages = [
-          ...config.packageOptions.external,
-          ...Object.keys(packageManifest.dependencies || {}),
-          ...Object.keys(packageManifest.peerDependencies || {}),
-        ].filter((ext) => ext !== _packageName && !NEVER_PEER_PACKAGES.includes(ext));
-        const externalPackagesFull = [
-          ...externalPackages,
-          ...Object.keys(packageManifest.devDependencies || {}).filter(
-            (ext) => ext !== _packageName && !NEVER_PEER_PACKAGES.includes(ext),
-          ),
-        ];
+
+        const filteredExternal = (external: string) => external !== _packageName && !NEVER_PEER_PACKAGES.has(external);
+
+        const dependenciesAndPeerDependencies = Object.keys(packageManifest.dependencies || {}).concat(
+          Object.keys(packageManifest.peerDependencies || {})
+        );
+        const devDependencies = Object.keys(packageManifest.devDependencies || {});
+
+        // Packages that should be marked as externalized. Any dependency
+        // or peerDependency that is not one of the packages we want to always bundle
+        const externalPackages = config.packageOptions.external.concat(
+          dependenciesAndPeerDependencies.filter(filteredExternal)
+        );
+
+        // The same as above, but includes devDependencies.
+        const externalPackagesFull = externalPackages.concat(
+          devDependencies.filter(filteredExternal)
+        );
 
         // To improve our ESM<>CJS conversion, we need to know the status of all dependencies.
         // This function returns a function, which can be used to fetch package.json manifests.

diff --git a/test/snowpack/config/packageOptions.external/index.test.js b/test/snowpack/config/packageOptions.external/index.test.js
@@ -64,4 +64,41 @@ describe('packageOptions.external', () => {
 
     expect(result['index.js']).toContain(`import 'some-thing/deep.js';`);
   });
+
+  it('will make node-fetch external if marked as external', async () => {
+    const result = await testFixture({
+      'packages/other/package.json': dedent`
+        {
+          "version": "1.0.0",
+          "name": "other",
+          "module": "main.js"
+        }
+      `,
+      'packages/other/main.js': dedent`
+        export default 'works';
+      `,
+      'package.json': dedent`
+        {
+          "version": "1.0.1",
+          "name": "@snowpack/test-config-external-node-fetch",
+          "dependencies": {
+            "other": "file:./packages/other"
+          }
+        }
+      `,
+      'index.js': dedent`
+        import 'node-fetch';
+        import 'other';
+      `,
+      'snowpack.config.js': dedent`
+        module.exports = {
+          packageOptions: {
+            external: ['node-fetch']
+          }
+        }
+      `
+    });
+
+    expect(result['index.js']).toContain(`import 'node-fetch';`);
+  });
 });