From ad78051b3572d419e872f8db801de06180809be6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bou=C3=A7as?= Date: Thu, 7 Sep 2023 14:01:28 +0100 Subject: [PATCH] fix: support import maps in npm module resolution (netlify/edge-bundler#471) * fix: support import maps in npm module resolution * fix: set default value of flag * refactor: add user-facing messages * fix: fix Windows paths * fix: fix capitalisation --- packages/edge-bundler/node/bundler.test.ts | 1 + packages/edge-bundler/node/bundler.ts | 21 ++- packages/edge-bundler/node/import_map.test.ts | 33 +++- packages/edge-bundler/node/import_map.ts | 32 ++++ .../edge-bundler/node/npm_dependencies.ts | 175 +++++++++++++----- packages/edge-bundler/shared/consts.ts | 1 + .../imports_npm_module/functions/func1.ts | 3 +- .../fixtures/imports_npm_module/helper.ts | 3 + .../imports_npm_module/import_map.json | 5 + 9 files changed, 215 insertions(+), 59 deletions(-) create mode 100644 packages/edge-bundler/test/fixtures/imports_npm_module/helper.ts create mode 100644 packages/edge-bundler/test/fixtures/imports_npm_module/import_map.json diff --git a/packages/edge-bundler/node/bundler.test.ts b/packages/edge-bundler/node/bundler.test.ts index 34e99499ff..a9dc0f3aaa 100644 --- a/packages/edge-bundler/node/bundler.test.ts +++ b/packages/edge-bundler/node/bundler.test.ts @@ -472,6 +472,7 @@ test('Loads npm modules from bare specifiers with and without the `npm:` prefix' await bundle([sourceDirectory], distPath, declarations, { basePath, featureFlags: { edge_functions_npm_modules: true }, + importMapPaths: [join(basePath, 'import_map.json')], vendorDirectory: vendorDirectory.path, }) const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8') diff --git a/packages/edge-bundler/node/bundler.ts b/packages/edge-bundler/node/bundler.ts index b771d42a01..33d9142f61 100644 --- a/packages/edge-bundler/node/bundler.ts +++ b/packages/edge-bundler/node/bundler.ts @@ -97,7 +97,14 @@ export const bundle = async ( const userFunctions = userSourceDirectories.length === 0 ? [] : await findFunctions(userSourceDirectories) const internalFunctions = internalSrcFolder ? await findFunctions([internalSrcFolder]) : [] const functions = [...internalFunctions, ...userFunctions] - const vendor = await safelyVendorNPMSpecifiers({ basePath, featureFlags, functions, logger, vendorDirectory }) + const vendor = await safelyVendorNPMSpecifiers({ + basePath, + featureFlags, + functions, + importMap, + logger, + vendorDirectory, + }) if (vendor) { importMap.add(vendor.importMap) @@ -239,6 +246,7 @@ interface VendorNPMOptions { basePath: string featureFlags: FeatureFlags functions: EdgeFunction[] + importMap: ImportMap logger: Logger vendorDirectory: string | undefined } @@ -247,6 +255,7 @@ const safelyVendorNPMSpecifiers = async ({ basePath, featureFlags, functions, + importMap, logger, vendorDirectory, }: VendorNPMOptions) => { @@ -255,11 +264,13 @@ const safelyVendorNPMSpecifiers = async ({ } try { - return await vendorNPMSpecifiers( + return await vendorNPMSpecifiers({ basePath, - functions.map(({ path }) => path), - vendorDirectory, - ) + directory: vendorDirectory, + functions: functions.map(({ path }) => path), + importMap, + logger, + }) } catch (error) { logger.system(error) } diff --git a/packages/edge-bundler/node/import_map.test.ts b/packages/edge-bundler/node/import_map.test.ts index 153465d95f..3d6daf6ce9 100644 --- a/packages/edge-bundler/node/import_map.test.ts +++ b/packages/edge-bundler/node/import_map.test.ts @@ -24,12 +24,20 @@ test('Handles import maps with full URLs without specifying a base URL', () => { } const map = new ImportMap([inputFile1, inputFile2]) - const { imports } = map.getContents() - expect(imports['netlify:edge']).toBe('https://edge.netlify.com/v1/index.ts?v=legacy') - expect(imports['@netlify/edge-functions']).toBe('https://edge.netlify.com/v1/index.ts') - expect(imports['alias:jamstack']).toBe('https://jamstack.org/') - expect(imports['alias:pets']).toBe('https://petsofnetlify.com/') + const m1 = map.getContents() + + expect(m1.imports['netlify:edge']).toBe('https://edge.netlify.com/v1/index.ts?v=legacy') + expect(m1.imports['@netlify/edge-functions']).toBe('https://edge.netlify.com/v1/index.ts') + expect(m1.imports['alias:jamstack']).toBe('https://jamstack.org/') + expect(m1.imports['alias:pets']).toBe('https://petsofnetlify.com/') + + const m2 = map.getContentsWithURLObjects() + + expect(m2.imports['netlify:edge']).toStrictEqual(new URL('https://edge.netlify.com/v1/index.ts?v=legacy')) + expect(m2.imports['@netlify/edge-functions']).toStrictEqual(new URL('https://edge.netlify.com/v1/index.ts')) + expect(m2.imports['alias:jamstack']).toStrictEqual(new URL('https://jamstack.org/')) + expect(m2.imports['alias:pets']).toStrictEqual(new URL('https://petsofnetlify.com/')) }) test('Resolves relative paths to absolute paths if a base path is not provided', () => { @@ -42,12 +50,19 @@ test('Resolves relative paths to absolute paths if a base path is not provided', } const map = new ImportMap([inputFile1]) - const { imports } = map.getContents() const expectedPath = join(cwd(), 'my-cool-site', 'heart', 'pets') - expect(imports['netlify:edge']).toBe('https://edge.netlify.com/v1/index.ts?v=legacy') - expect(imports['@netlify/edge-functions']).toBe('https://edge.netlify.com/v1/index.ts') - expect(imports['alias:pets']).toBe(`${pathToFileURL(expectedPath).toString()}/`) + const m1 = map.getContents() + + expect(m1.imports['netlify:edge']).toBe('https://edge.netlify.com/v1/index.ts?v=legacy') + expect(m1.imports['@netlify/edge-functions']).toBe('https://edge.netlify.com/v1/index.ts') + expect(m1.imports['alias:pets']).toBe(`${pathToFileURL(expectedPath).toString()}/`) + + const m2 = map.getContentsWithURLObjects() + + expect(m2.imports['netlify:edge']).toStrictEqual(new URL('https://edge.netlify.com/v1/index.ts?v=legacy')) + expect(m2.imports['@netlify/edge-functions']).toStrictEqual(new URL('https://edge.netlify.com/v1/index.ts')) + expect(m2.imports['alias:pets']).toStrictEqual(new URL(`${pathToFileURL(expectedPath).toString()}/`)) }) describe('Returns the fully resolved import map', () => { diff --git a/packages/edge-bundler/node/import_map.ts b/packages/edge-bundler/node/import_map.ts index 1ba1c2e44b..4a087a6814 100644 --- a/packages/edge-bundler/node/import_map.ts +++ b/packages/edge-bundler/node/import_map.ts @@ -76,6 +76,26 @@ export class ImportMap { ) } + static convertImportsToURLObjects(imports: Imports) { + return Object.entries(imports).reduce( + (acc, [key, value]) => ({ + ...acc, + [key]: new URL(value), + }), + {} as Record, + ) + } + + static convertScopesToURLObjects(scopes: Record) { + return Object.entries(scopes).reduce( + (acc, [key, value]) => ({ + ...acc, + [key]: ImportMap.convertImportsToURLObjects(value), + }), + {} as Record>, + ) + } + // Applies a list of prefixes to a given path, returning the replaced path. // For example, given a `path` of `file:///foo/bar/baz.js` and a `prefixes` // object with `{"file:///foo/": "file:///hello/"}`, this method will return @@ -180,6 +200,18 @@ export class ImportMap { } } + // The same as `getContents`, but the URLs are represented as URL objects + // instead of strings. This is compatible with the `ParsedImportMap` type + // from the `@import-maps/resolve` library. + getContentsWithURLObjects(prefixes: Record = {}) { + const { imports, scopes } = this.getContents(prefixes) + + return { + imports: ImportMap.convertImportsToURLObjects(imports), + scopes: ImportMap.convertScopesToURLObjects(scopes), + } + } + static async readFile(path: string, logger: Logger): Promise { const baseURL = pathToFileURL(path) diff --git a/packages/edge-bundler/node/npm_dependencies.ts b/packages/edge-bundler/node/npm_dependencies.ts index f36a5b6cd8..d40fa6c51d 100644 --- a/packages/edge-bundler/node/npm_dependencies.ts +++ b/packages/edge-bundler/node/npm_dependencies.ts @@ -1,17 +1,40 @@ import { promises as fs } from 'fs' -import { builtinModules } from 'module' +import { builtinModules, createRequire } from 'module' import path from 'path' -import { pathToFileURL } from 'url' +import { fileURLToPath, pathToFileURL } from 'url' -import { build, Plugin } from 'esbuild' +import { resolve, ParsedImportMap } from '@import-maps/resolve' +import { build, OnResolveResult, Plugin } from 'esbuild' import tmp from 'tmp-promise' -import { npmPrefix } from '../shared/consts.js' +import { nodePrefix, npmPrefix } from '../shared/consts.js' + +import { ImportMap } from './import_map.js' +import { Logger } from './logger.js' + +const builtinModulesSet = new Set(builtinModules) +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"; + let __filename=___nfyFileURLToPath(import.meta.url); + let __dirname=___nfyPathDirname(___nfyFileURLToPath(import.meta.url)); + let require=___nfyCreateRequire(import.meta.url); + `, +} // esbuild plugin that will traverse the code and look for imports of external // dependencies (i.e. Node modules). It stores the specifiers found in the Set // provided. -export const getDependencyTrackerPlugin = (specifiers: Set): Plugin => ({ +export const getDependencyTrackerPlugin = ( + specifiers: Set, + importMap: ParsedImportMap, + baseURL: URL, +): Plugin => ({ name: 'dependency-tracker', setup(build) { build.onResolve({ filter: /^(.*)$/ }, (args) => { @@ -19,10 +42,31 @@ export const getDependencyTrackerPlugin = (specifiers: Set): Plugin => ( return } + const result: Partial = {} + + let specifier = args.path + + // Start by checking whether the specifier matches any import map defined + // by the user. + const { matched, resolvedImport } = resolve(specifier, importMap, baseURL) + + // If it does, the resolved import is the specifier we'll evaluate going + // forward. + if (matched) { + specifier = fileURLToPath(resolvedImport).replace(/\\/g, '/') + + result.path = specifier + } + + // If the specifier is a Node.js built-in, we don't want to bundle it. + if (specifier.startsWith(nodePrefix) || builtinModulesSet.has(specifier)) { + return { external: true } + } + // If the specifier has the `npm:` prefix, strip it and use the rest of // the specifier to resolve the module. - if (args.path.startsWith(npmPrefix)) { - const canonicalPath = args.path.slice(npmPrefix.length) + if (specifier.startsWith(npmPrefix)) { + const canonicalPath = specifier.slice(npmPrefix.length) return build.resolve(canonicalPath, { kind: args.kind, @@ -30,30 +74,58 @@ export const getDependencyTrackerPlugin = (specifiers: Set): Plugin => ( }) } - const isLocalImport = args.path.startsWith(path.sep) || args.path.startsWith('.') + const isLocalImport = specifier.startsWith(path.sep) || specifier.startsWith('.') + // If this is a local import, return so that esbuild visits that path. if (isLocalImport) { - return + return result } - const isRemoteURLImport = args.path.startsWith('https://') || args.path.startsWith('http://') + const isRemoteURLImport = specifier.startsWith('https://') || specifier.startsWith('http://') + + if (isRemoteURLImport) { + return { external: true } + } - if (!isRemoteURLImport) { - specifiers.add(args.path) + // At this point we know we're dealing with a bare specifier that should + // be treated as an external module. We first try to resolve it, because + // in the event that it doesn't exist (e.g. user is referencing a module + // that they haven't installed) we won't even attempt to bundle it. This + // lets the ESZIP bundler handle and report the missing import instead of + // esbuild, which is a better experience for the user. + try { + require.resolve(specifier, { paths: [args.resolveDir] }) + + specifiers.add(specifier) + } catch { + // no-op } - // At this point we know we're not dealing with a local import, so we're - // about to leave the boundaries of user code. We mark the specifier as - // external, because we're not interested in traversing the entire module - // tree — i.e. if user code imports module `foo` and that imports `bar`, - // we only want to add `foo` to the list of specifiers, since the whole - // module — including its dependencies like `bar` — will be bundled. + // Mark the specifier as external, because we don't want to traverse the + // entire module tree — i.e. if user code imports module `foo` and that + // imports `bar`, we only want to add `foo` to the list of specifiers, + // since the whole module — including its dependencies like `bar` — + // will be bundled. return { external: true } }) }, }) -export const vendorNPMSpecifiers = async (basePath: string, functions: string[], directory?: string) => { +interface VendorNPMSpecifiersOptions { + basePath: string + directory?: string + functions: string[] + importMap: ImportMap + logger: Logger +} + +export const vendorNPMSpecifiers = async ({ + basePath, + directory, + functions, + importMap, + logger, +}: VendorNPMSpecifiersOptions) => { const specifiers = new Set() // The directories that esbuild will use when resolving Node modules. We must @@ -62,34 +134,48 @@ export const vendorNPMSpecifiers = async (basePath: string, functions: string[], // resolution logic won't work. 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. + const temporaryDirectory = directory ? { path: directory } : await tmp.dir() + // Do a first pass at bundling to gather a list of specifiers that should be // loaded as npm dependencies, because they either use the `npm:` prefix or // they are bare specifiers. We'll collect them in `specifiers`. - await build({ - bundle: true, - entryPoints: functions, - nodePaths, - platform: 'node', - plugins: [getDependencyTrackerPlugin(specifiers)], - write: false, - }) + try { + await build({ + banner, + bundle: true, + entryPoints: functions, + logLevel: 'error', + nodePaths, + outdir: temporaryDirectory.path, + platform: 'node', + plugins: [getDependencyTrackerPlugin(specifiers, importMap.getContentsWithURLObjects(), pathToFileURL(basePath))], + write: false, + }) + } catch (error) { + logger.system('Could not track dependencies in edge function:', error) + logger.user( + 'An error occurred when trying to scan your edge functions for npm modules, which is an experimental feature. If you are loading npm modules, please share the errors above in https://ntl.fyi/edge-functions-npm. If you are not loading npm modules, you can ignore this message.', + ) + } // If we found no specifiers, there's nothing left to do here. if (specifiers.size === 0) { return } - // 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. - const temporaryDirectory = directory ? { path: directory } : await tmp.dir() + logger.user( + '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 // 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. const ops = await Promise.all( [...specifiers].map(async (specifier, index) => { - const code = `export { default } from "${specifier}"; export * from "${specifier}";` + const code = `import * as mod from "${specifier}"; export default mod.default; export * from "${specifier}";` const filePath = path.join(temporaryDirectory.path, `stub-${index}.js`) await fs.writeFile(filePath, code) @@ -104,9 +190,11 @@ export const vendorNPMSpecifiers = async (basePath: string, functions: string[], // stubs (such that a common module isn't bundled twice). await build({ allowOverwrite: true, + banner, bundle: true, entryPoints, format: 'esm', + logLevel: 'error', nodePaths, outdir: temporaryDirectory.path, platform: 'node', @@ -129,18 +217,17 @@ export const vendorNPMSpecifiers = async (basePath: string, functions: string[], // map, mapping specifiers to the paths of their bundled files on disk. Each // specifier gets two entries in the import map, one with the `npm:` prefix // and one without, such that both options are supported. - const imports = ops.reduce((acc, op) => { - const url = pathToFileURL(op.filePath).toString() - - return { - ...acc, - [op.specifier]: url, - [npmPrefix + op.specifier]: url, - } - }, builtIns) - const importMap = { + const newImportMap = { baseURL: pathToFileURL(temporaryDirectory.path), - imports, + imports: ops.reduce((acc, op) => { + const url = pathToFileURL(op.filePath).toString() + + return { + ...acc, + [op.specifier]: url, + [npmPrefix + op.specifier]: url, + } + }, builtIns), } const cleanup = async () => { @@ -160,6 +247,6 @@ export const vendorNPMSpecifiers = async (basePath: string, functions: string[], return { cleanup, directory: temporaryDirectory.path, - importMap, + importMap: newImportMap, } } diff --git a/packages/edge-bundler/shared/consts.ts b/packages/edge-bundler/shared/consts.ts index 293671d28b..1307adb2c7 100644 --- a/packages/edge-bundler/shared/consts.ts +++ b/packages/edge-bundler/shared/consts.ts @@ -1,4 +1,5 @@ export const importMapSpecifier = 'netlify:import-map' +export const nodePrefix = 'node:' export const npmPrefix = 'npm:' export const virtualRoot = 'file:///root/' export const virtualVendorRoot = 'file:///vendor/' diff --git a/packages/edge-bundler/test/fixtures/imports_npm_module/functions/func1.ts b/packages/edge-bundler/test/fixtures/imports_npm_module/functions/func1.ts index 7de2537d97..d7f34a7279 100644 --- a/packages/edge-bundler/test/fixtures/imports_npm_module/functions/func1.ts +++ b/packages/edge-bundler/test/fixtures/imports_npm_module/functions/func1.ts @@ -1,9 +1,10 @@ import parent1 from 'parent-1' import parent2 from 'npm:parent-2' import parent3 from './lib/util.ts' +import { echo } from 'alias:helper' export default async () => { const text = [parent1('JavaScript'), parent2('APIs'), parent3('Markup')].join(', ') - return new Response(text) + return new Response(echo(text)) } diff --git a/packages/edge-bundler/test/fixtures/imports_npm_module/helper.ts b/packages/edge-bundler/test/fixtures/imports_npm_module/helper.ts new file mode 100644 index 0000000000..0415c46e50 --- /dev/null +++ b/packages/edge-bundler/test/fixtures/imports_npm_module/helper.ts @@ -0,0 +1,3 @@ +export const greet = (name: string) => `Hello, ${name}!` +export const echo = (name: string) => name +export const yell = (message: string) => message.toUpperCase() diff --git a/packages/edge-bundler/test/fixtures/imports_npm_module/import_map.json b/packages/edge-bundler/test/fixtures/imports_npm_module/import_map.json new file mode 100644 index 0000000000..a72e1a7d9f --- /dev/null +++ b/packages/edge-bundler/test/fixtures/imports_npm_module/import_map.json @@ -0,0 +1,5 @@ +{ + "imports": { + "alias:helper": "./helper.ts" + } +}