feat: remove support for npm: prefix (#472)

* feat: remove support for `npm:` prefix

* feat: show better error message

* refactor: stub -> barrel

* chore: update comment

node/bundler.test.ts

@@ -125,7 +125,7 @@ test('Adds a custom error property to user errors during bundling', async () =>
   }
 })
 
-test('Prints a nice error message when user tries importing NPM module', async () => {
+test('Prints a nice error message when user tries importing an npm module and npm support is disabled', async () => {
   expect.assertions(2)
 
   const { basePath, cleanup, distPath } = await useFixture('imports_npm_module')
@@ -142,7 +142,34 @@ test('Prints a nice error message when user tries importing NPM module', async (
   } catch (error) {
     expect(error).toBeInstanceOf(BundleError)
     expect((error as BundleError).message).toEqual(
-      `It seems like you're trying to import an npm module. This is only supported via CDNs like esm.sh. Have you tried 'import mod from "https://esm.sh/parent-1"'?`,
+      `It seems like you're trying to import an npm module. This is only supported via CDNs like esm.sh. Have you tried 'import mod from "https://esm.sh/parent-2"'?`,
+    )
+  } finally {
+    await cleanup()
+  }
+})
+
+test('Prints a nice error message when user tries importing an npm module and npm support is enabled', async () => {
+  expect.assertions(2)
+
+  const { basePath, cleanup, distPath } = await useFixture('imports_npm_module_scheme')
+  const sourceDirectory = join(basePath, 'functions')
+  const declarations: Declaration[] = [
+    {
+      function: 'func1',
+      path: '/func1',
+    },
+  ]
+
+  try {
+    await bundle([sourceDirectory], distPath, declarations, {
+      basePath,
+      featureFlags: { edge_functions_npm_modules: true },
+    })
+  } catch (error) {
+    expect(error).toBeInstanceOf(BundleError)
+    expect((error as BundleError).message).toEqual(
+      `There was an error when loading the 'p-retry' npm module. Support for npm modules in edge functions is an experimental feature. Refer to https://ntl.fyi/edge-functions-npm for more information.`,
     )
   } finally {
     await cleanup()
@@ -458,7 +485,7 @@ test('Handles imports with the `node:` prefix', async () => {
   await cleanup()
 })
 
-test('Loads npm modules from bare specifiers with and without the `npm:` prefix', async () => {
+test('Loads npm modules from bare specifiers', async () => {
   const { basePath, cleanup, distPath } = await useFixture('imports_npm_module')
   const sourceDirectory = join(basePath, 'functions')
   const declarations: Declaration[] = [

node/formats/eszip.ts

@@ -33,6 +33,7 @@ const bundleESZIP = async ({
   deno,
   distDirectory,
   externals,
+  featureFlags,
   functions,
   importMap,
   vendorDirectory,
@@ -66,7 +67,9 @@ const bundleESZIP = async ({
   try {
     await deno.run(['run', ...flags, bundler, JSON.stringify(payload)], { pipeOutput: true })
   } catch (error: unknown) {
-    throw wrapBundleError(wrapNpmImportError(error), { format: 'eszip' })
+    throw wrapBundleError(wrapNpmImportError(error, Boolean(featureFlags.edge_functions_npm_modules)), {
+      format: 'eszip',
+    })
   }
 
   const hash = await getFileHash(destPath)

node/npm_dependencies.ts

@@ -18,9 +18,9 @@ const require = createRequire(import.meta.url)
 // Workaround for https://github.com/evanw/esbuild/issues/1921.
 const banner = {
   js: `
-  import {createRequire as ___nfyCreateRequire} from "module";
-  import {fileURLToPath as ___nfyFileURLToPath} from "url";
-  import {dirname as ___nfyPathDirname} from "path";
+  import {createRequire as ___nfyCreateRequire} from "node:module";
+  import {fileURLToPath as ___nfyFileURLToPath} from "node:url";
+  import {dirname as ___nfyPathDirname} from "node:path";
   let __filename=___nfyFileURLToPath(import.meta.url);
   let __dirname=___nfyPathDirname(___nfyFileURLToPath(import.meta.url));
   let require=___nfyCreateRequire(import.meta.url);
@@ -63,15 +63,10 @@ export const getDependencyTrackerPlugin = (
         return { external: true }
       }
 
-      // If the specifier has the `npm:` prefix, strip it and use the rest of
-      // the specifier to resolve the module.
+      // We don't support the `npm:` prefix yet. Mark the specifier as external
+      // and the ESZIP bundler will handle the failure.
       if (specifier.startsWith(npmPrefix)) {
-        const canonicalPath = specifier.slice(npmPrefix.length)
-
-        return build.resolve(canonicalPath, {
-          kind: args.kind,
-          resolveDir: args.resolveDir,
-        })
+        return { external: true }
       }
 
       const isLocalImport = specifier.startsWith(path.sep) || specifier.startsWith('.')
@@ -135,8 +130,8 @@ export const vendorNPMSpecifiers = async ({
   const nodePaths = [path.join(basePath, 'node_modules')]
 
   // We need to create some files on disk, which we don't want to write to the
-  // project directory. If a custom directory has been specified, which happens
-  // only in tests, we use it. Otherwise, create a random temporary directory.
+  // project directory. If a custom directory has been specified, we use it.
+  // Otherwise, create a random temporary directory.
   const temporaryDirectory = directory ? { path: directory } : await tmp.dir()
 
   // Do a first pass at bundling to gather a list of specifiers that should be
@@ -170,13 +165,13 @@ export const vendorNPMSpecifiers = async ({
     'You are using npm modules in Edge Functions, which is an experimental feature. Learn more at https://ntl.fyi/edge-functions-npm.',
   )
 
-  // To bundle an entire module and all its dependencies, we create a stub file
+  // To bundle an entire module and all its dependencies, create a barrel file
   // where we re-export everything from that specifier. We do this for every
-  // specifier, and each of these files will be the entry points to esbuild.
+  // specifier, and each of these files will become entry points to esbuild.
   const ops = await Promise.all(
     [...specifiers].map(async (specifier, index) => {
       const code = `import * as mod from "${specifier}"; export default mod.default; export * from "${specifier}";`
-      const filePath = path.join(temporaryDirectory.path, `stub-${index}.js`)
+      const filePath = path.join(temporaryDirectory.path, `barrel-${index}.js`)
 
       await fs.writeFile(filePath, code)
 
@@ -185,9 +180,9 @@ export const vendorNPMSpecifiers = async ({
   )
   const entryPoints = ops.map(({ filePath }) => filePath)
 
-  // Bundle each of the stub files we've created. We'll end up with a compiled
-  // version of each of the stub files, plus any chunks of shared code between
-  // stubs (such that a common module isn't bundled twice).
+  // Bundle each of the barrel files we created. We'll end up with a compiled
+  // version of each of the barrel files, plus any chunks of shared code
+  // between them (such that a common module isn't bundled twice).
   await build({
     allowOverwrite: true,
     banner,
@@ -225,7 +220,6 @@ export const vendorNPMSpecifiers = async ({
       return {
         ...acc,
         [op.specifier]: url,
-        [npmPrefix + op.specifier]: url,
       }
     }, builtIns),
   }

node/npm_import_error.ts

@@ -1,8 +1,12 @@
 class NPMImportError extends Error {
-  constructor(originalError: Error, moduleName: string) {
-    super(
-      `It seems like you're trying to import an npm module. This is only supported via CDNs like esm.sh. Have you tried 'import mod from "https://esm.sh/${moduleName}"'?`,
-    )
+  constructor(originalError: Error, moduleName: string, supportsNPM: boolean) {
+    let message = `It seems like you're trying to import an npm module. This is only supported via CDNs like esm.sh. Have you tried 'import mod from "https://esm.sh/${moduleName}"'?`
+
+    if (supportsNPM) {
+      message = `There was an error when loading the '${moduleName}' npm module. Support for npm modules in edge functions is an experimental feature. Refer to https://ntl.fyi/edge-functions-npm for more information.`
+    }
+
+    super(message)
 
     this.name = 'NPMImportError'
     this.stack = originalError.stack
@@ -12,18 +16,18 @@ class NPMImportError extends Error {
   }
 }
 
-const wrapNpmImportError = (input: unknown) => {
+const wrapNpmImportError = (input: unknown, supportsNPM: boolean) => {
   if (input instanceof Error) {
     const match = input.message.match(/Relative import path "(.*)" not prefixed with/)
     if (match !== null) {
       const [, moduleName] = match
-      return new NPMImportError(input, moduleName)
+      return new NPMImportError(input, moduleName, supportsNPM)
     }
 
     const schemeMatch = input.message.match(/Error: Module not found "npm:(.*)"/)
     if (schemeMatch !== null) {
       const [, moduleName] = schemeMatch
-      return new NPMImportError(input, moduleName)
+      return new NPMImportError(input, moduleName, supportsNPM)
     }
   }
 

test/fixtures/imports_npm_module/functions/func1.ts

@@ -1,5 +1,5 @@
 import parent1 from 'parent-1'
-import parent2 from 'npm:parent-2'
+import parent2 from 'parent-2'
 import parent3 from './lib/util.ts'
 import { echo } from 'alias:helper'