Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove npm_modules and fail_unsupported_regex flags #514

Merged
merged 3 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 1 addition & 54 deletions node/bundler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,34 +125,7 @@ test('Adds a custom error property to user errors during bundling', 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')
const sourceDirectory = join(basePath, 'functions')
const declarations: Declaration[] = [
{
function: 'func1',
path: '/func1',
},
]

try {
await bundle([sourceDirectory], distPath, declarations, {
basePath,
importMapPaths: [join(basePath, 'import_map.json')],
})
} 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"'?`,
)
} finally {
await cleanup()
}
})

test('Prints a nice error message when user tries importing an npm module and npm support is enabled', async () => {
test('Prints a nice error message when user tries importing an npm module', async () => {
expect.assertions(2)

const { basePath, cleanup, distPath } = await useFixture('imports_npm_module_scheme')
Expand All @@ -167,7 +140,6 @@ test('Prints a nice error message when user tries importing an npm module and np
try {
await bundle([sourceDirectory], distPath, declarations, {
basePath,
featureFlags: { edge_functions_npm_modules: true },
})
} catch (error) {
expect(error).toBeInstanceOf(BundleError)
Expand All @@ -179,30 +151,6 @@ test('Prints a nice error message when user tries importing an npm module and np
}
})

test('Prints a nice error message when user tries importing NPM module with npm: scheme', 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 })
} 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/p-retry"'?`,
)
} finally {
await cleanup()
}
})

test('Does not add a custom error property to system errors during bundling', async () => {
expect.assertions(1)

Expand Down Expand Up @@ -502,7 +450,6 @@ test('Loads npm modules from bare specifiers', async () => {

await bundle([sourceDirectory], distPath, declarations, {
basePath,
featureFlags: { edge_functions_npm_modules: true },
importMapPaths: [join(basePath, 'import_map.json')],
vendorDirectory: vendorDirectory.path,
systemLogger,
Expand Down
5 changes: 0 additions & 5 deletions node/bundler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,16 +255,11 @@ interface VendorNPMOptions {

const safelyVendorNPMSpecifiers = async ({
basePath,
featureFlags,
functions,
importMap,
logger,
vendorDirectory,
}: VendorNPMOptions) => {
if (!featureFlags.edge_functions_npm_modules) {
return
}

try {
return await vendorNPMSpecifiers({
basePath,
Expand Down
5 changes: 1 addition & 4 deletions node/feature_flags.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
const defaultFlags = {
edge_functions_fail_unsupported_regex: false,
edge_functions_npm_modules: false,
}
const defaultFlags = {}

type FeatureFlag = keyof typeof defaultFlags
type FeatureFlags = Partial<Record<FeatureFlag, boolean>>
Expand Down
3 changes: 1 addition & 2 deletions node/formats/eszip.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ const bundleESZIP = async ({
deno,
distDirectory,
externals,
featureFlags,
functions,
importMap,
vendorDirectory,
Expand Down Expand Up @@ -67,7 +66,7 @@ const bundleESZIP = async ({
try {
await deno.run(['run', ...flags, bundler, JSON.stringify(payload)], { pipeOutput: true })
} catch (error: unknown) {
throw wrapBundleError(wrapNpmImportError(error, Boolean(featureFlags.edge_functions_npm_modules)), {
throw wrapBundleError(wrapNpmImportError(error), {
format: 'eszip',
})
}
Expand Down
28 changes: 2 additions & 26 deletions node/manifest.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { env } from 'process'

import { test, expect, vi } from 'vitest'
import { test, expect } from 'vitest'

import { getRouteMatcher } from '../test/util.js'

Expand Down Expand Up @@ -433,38 +433,14 @@ test('Generates a manifest with layers', () => {
expect(manifest2.layers).toEqual(layers)
})

test('Shows a warning if the regular expression contains a negative lookahead', () => {
const mockConsoleWarn = vi.fn()
const consoleWarn = console.warn

console.warn = mockConsoleWarn

const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', pattern: '^/\\w+(?=\\d)$' }]
const manifest = generateManifest({
bundles: [],
declarations,
functions,
})

console.warn = consoleWarn

expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/\\w+(?=\\d)$', excluded_patterns: [] }])
expect(mockConsoleWarn).toHaveBeenCalledOnce()
expect(mockConsoleWarn).toHaveBeenCalledWith(
"Function 'func-1' uses an unsupported regular expression and will not be invoked: Regular expressions with lookaheads are not supported",
)
})

test('Throws an error if the regular expression contains a negative lookahead and the `edge_functions_fail_unsupported_regex` flag is set', () => {
test('Throws an error if the regular expression contains a negative lookahead', () => {
const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', pattern: '^/\\w+(?=\\d)$' }]

expect(() =>
generateManifest({
bundles: [],
declarations,
featureFlags: { edge_functions_fail_unsupported_regex: true },
functions,
}),
).toThrowError(
Expand Down
38 changes: 10 additions & 28 deletions node/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ const normalizeMethods = (method: unknown, name: string): string[] | undefined =
const generateManifest = ({
bundles = [],
declarations = [],
featureFlags,
functions,
userFunctionConfig = {},
internalFunctionConfig = {},
Expand Down Expand Up @@ -148,8 +147,8 @@ const generateManifest = ({
return
}

const pattern = getRegularExpression(declaration, featureFlags)
const excludedPattern = getExcludedRegularExpressions(declaration, featureFlags)
const pattern = getRegularExpression(declaration)
const excludedPattern = getExcludedRegularExpressions(declaration)

const route: Route = {
function: func.name,
Expand Down Expand Up @@ -207,32 +206,23 @@ const pathToRegularExpression = (path: string) => {
}
}

const getRegularExpression = (declaration: Declaration, featureFlags?: FeatureFlags): string => {
const getRegularExpression = (declaration: Declaration): string => {
if ('pattern' in declaration) {
try {
return parsePattern(declaration.pattern)
} catch (error: unknown) {
// eslint-disable-next-line max-depth
if (featureFlags?.edge_functions_fail_unsupported_regex) {
throw new Error(
throw wrapBundleError(
new Error(
`Could not parse path declaration of function '${declaration.function}': ${(error as Error).message}`,
)
}

console.warn(
`Function '${declaration.function}' uses an unsupported regular expression and will not be invoked: ${
(error as Error).message
}`,
),
)

return declaration.pattern
}
}

return pathToRegularExpression(declaration.path)
}

const getExcludedRegularExpressions = (declaration: Declaration, featureFlags?: FeatureFlags): string[] => {
const getExcludedRegularExpressions = (declaration: Declaration): string[] => {
if ('excludedPattern' in declaration && declaration.excludedPattern) {
const excludedPatterns: string[] = Array.isArray(declaration.excludedPattern)
? declaration.excludedPattern
Expand All @@ -241,19 +231,11 @@ const getExcludedRegularExpressions = (declaration: Declaration, featureFlags?:
try {
return parsePattern(excludedPattern)
} catch (error: unknown) {
if (featureFlags?.edge_functions_fail_unsupported_regex) {
throw new Error(
throw wrapBundleError(
new Error(
`Could not parse path declaration of function '${declaration.function}': ${(error as Error).message}`,
)
}

console.warn(
`Function '${
declaration.function
}' uses an unsupported regular expression and will therefore not be invoked: ${(error as Error).message}`,
),
)

return excludedPattern
}
})
}
Expand Down
18 changes: 7 additions & 11 deletions node/npm_import_error.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
class NPMImportError extends Error {
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)
constructor(originalError: Error, moduleName: string) {
super(
`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.`,
)

this.name = 'NPMImportError'
this.stack = originalError.stack
Expand All @@ -16,18 +12,18 @@ class NPMImportError extends Error {
}
}

const wrapNpmImportError = (input: unknown, supportsNPM: boolean) => {
const wrapNpmImportError = (input: unknown) => {
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, supportsNPM)
return new NPMImportError(input, moduleName)
}

const schemeMatch = input.message.match(/Error: Module not found "npm:(.*)"/)
if (schemeMatch !== null) {
const [, moduleName] = schemeMatch
return new NPMImportError(input, moduleName, supportsNPM)
return new NPMImportError(input, moduleName)
}
}

Expand Down
3 changes: 0 additions & 3 deletions node/server/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ test('Starts a server and serves requests for edge functions', async () => {
importMapPaths,
port,
servePath,
featureFlags: {
edge_functions_npm_modules: true,
},
})

const functions = [
Expand Down
29 changes: 13 additions & 16 deletions node/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ const prepareServer = ({
deno,
distDirectory,
distImportMapPath,
featureFlags,
flags: denoFlags,
formatExportTypeError,
formatImportError,
Expand Down Expand Up @@ -71,21 +70,19 @@ const prepareServer = ({
const importMap = baseImportMap.clone()
const npmSpecifiersWithExtraneousFiles: string[] = []

if (featureFlags?.edge_functions_npm_modules) {
const vendor = await vendorNPMSpecifiers({
basePath,
directory: distDirectory,
functions: functions.map(({ path }) => path),
importMap,
logger,
referenceTypes: true,
})

if (vendor) {
features.npmModules = true
importMap.add(vendor.importMap)
npmSpecifiersWithExtraneousFiles.push(...vendor.npmSpecifiersWithExtraneousFiles)
}
const vendor = await vendorNPMSpecifiers({
basePath,
directory: distDirectory,
functions: functions.map(({ path }) => path),
importMap,
logger,
referenceTypes: true,
})

if (vendor) {
features.npmModules = true
importMap.add(vendor.importMap)
npmSpecifiersWithExtraneousFiles.push(...vendor.npmSpecifiersWithExtraneousFiles)
}

try {
Expand Down