From 32a73871e4aa1b07bd04227ae22e50e143280095 Mon Sep 17 00:00:00 2001 From: Daniel Tschinder <231804+danez@users.noreply.github.com> Date: Thu, 9 Jun 2022 16:45:48 +0100 Subject: [PATCH] fix: detect a wider range of scheduled functions (#1105) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: detect a wider range of scheduled functions scheduled functions which are reassigned or not directly exported are now detected * Update src/runtimes/node/parser/bindings.ts Co-authored-by: Eduardo Bouças * Update src/runtimes/node/parser/exports.ts Co-authored-by: Eduardo Bouças * Update src/runtimes/node/parser/exports.ts Co-authored-by: Eduardo Bouças * Apply suggestions from code review Co-authored-by: Eduardo Bouças * refactor: clean up code Co-authored-by: Eduardo Bouças --- src/runtimes/node/in_source_config/index.ts | 6 +- .../in_source_config/properties/schedule.ts | 13 ++++- src/runtimes/node/parser/bindings.ts | 56 ++++++++++++++++++ src/runtimes/node/parser/exports.ts | 58 +++++++++++++++---- .../functions/cron_esm_no_direct_export.js | 7 +++ .../cron_esm_no_direct_export_renamed.js | 6 ++ ...esm_no_direct_export_renamed_reassigned.js | 8 +++ .../functions/cron_esm_schedule_reassigned.js | 7 +++ tests/main.js | 4 +- 9 files changed, 147 insertions(+), 18 deletions(-) create mode 100644 src/runtimes/node/parser/bindings.ts create mode 100644 tests/fixtures/in-source-config/functions/cron_esm_no_direct_export.js create mode 100644 tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed.js create mode 100644 tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed_reassigned.js create mode 100644 tests/fixtures/in-source-config/functions/cron_esm_schedule_reassigned.js diff --git a/src/runtimes/node/in_source_config/index.ts b/src/runtimes/node/in_source_config/index.ts index 338fd185d..059985a4c 100644 --- a/src/runtimes/node/in_source_config/index.ts +++ b/src/runtimes/node/in_source_config/index.ts @@ -1,6 +1,7 @@ import { ArgumentPlaceholder, Expression, SpreadElement, JSXNamespacedName } from '@babel/types' import { nonNullable } from '../../../utils/non_nullable.js' +import { createBindingsMethod } from '../parser/bindings.js' import { getMainExport } from '../parser/exports.js' import { getImports } from '../parser/imports.js' import { safelyParseFile } from '../parser/index.js' @@ -22,7 +23,8 @@ export const findISCDeclarationsInPath = async (sourcePath: string): Promise getImports(node, IN_SOURCE_CONFIG_MODULE)) - const mainExports = getMainExport(ast.body) + const getAllBindings = createBindingsMethod(ast.body) + const mainExports = getMainExport(ast.body, getAllBindings) const iscExports = mainExports .map(({ args, local: exportName }) => { const matchingImport = imports.find(({ local: importName }) => importName === exportName) @@ -33,7 +35,7 @@ export const findISCDeclarationsInPath = async (sourcePath: string): Promise { - const [expression] = args +export const parse = ({ args }: { args: ISCHandlerArg[] }, getAllBindings: BindingMethod) => { + let [expression] = args + + if (expression.type === 'Identifier') { + const binding = getAllBindings().get(expression.name) + if (binding) { + expression = binding + } + } + const schedule = expression.type === 'StringLiteral' ? expression.value : undefined return { diff --git a/src/runtimes/node/parser/bindings.ts b/src/runtimes/node/parser/bindings.ts new file mode 100644 index 000000000..09d9c0bfa --- /dev/null +++ b/src/runtimes/node/parser/bindings.ts @@ -0,0 +1,56 @@ +import type { Expression, Statement, VariableDeclaration } from '@babel/types' + +type Bindings = Map + +const getBindingFromVariableDeclaration = function (node: VariableDeclaration, bindings: Bindings): void { + node.declarations.forEach((declaration) => { + if (declaration.id.type === 'Identifier' && declaration.init) { + bindings.set(declaration.id.name, declaration.init) + } + }) +} + +// eslint-disable-next-line complexity +const getBindingsFromNode = function (node: Statement, bindings: Bindings): void { + if (node.type === 'VariableDeclaration') { + // A variable was created, so create it and store the potential value + getBindingFromVariableDeclaration(node, bindings) + } else if ( + node.type === 'ExpressionStatement' && + node.expression.type === 'AssignmentExpression' && + node.expression.left.type === 'Identifier' + ) { + // The variable was reassigned, so let's store the new value + bindings.set(node.expression.left.name, node.expression.right) + } else if (node.type === 'ExportNamedDeclaration' && node.declaration?.type === 'VariableDeclaration') { + // A `export const|let ...` creates a binding that can later be referenced again + getBindingFromVariableDeclaration(node.declaration, bindings) + } +} + +/** + * Goes through all relevant nodes and creates a map from binding name to assigned value/expression + */ +const getAllBindings = function (nodes: Statement[]): Bindings { + const bindings: Bindings = new Map() + + nodes.forEach((node) => { + getBindingsFromNode(node, bindings) + }) + + return bindings +} + +export type BindingMethod = () => Bindings + +export const createBindingsMethod = function (nodes: Statement[]): BindingMethod { + // memoize the result for these nodes + let result: Bindings + return () => { + if (!result) { + result = getAllBindings(nodes) + } + + return result + } +} diff --git a/src/runtimes/node/parser/exports.ts b/src/runtimes/node/parser/exports.ts index cfd30a8e2..cc5f17cc4 100644 --- a/src/runtimes/node/parser/exports.ts +++ b/src/runtimes/node/parser/exports.ts @@ -1,15 +1,16 @@ -import { CallExpression, Statement } from '@babel/types' +import type { ExportNamedDeclaration, ExportSpecifier, Expression, Statement } from '@babel/types' import type { ISCExport } from '../in_source_config/index.js' +import type { BindingMethod } from './bindings.js' import { isModuleExports } from './helpers.js' // Finds the main handler export in an AST. -export const getMainExport = (nodes: Statement[]) => { +export const getMainExport = (nodes: Statement[], getAllBindings: BindingMethod) => { let handlerExport: ISCExport[] = [] nodes.find((node) => { - const esmExports = getMainExportFromESM(node) + const esmExports = getMainExportFromESM(node, getAllBindings) if (esmExports.length !== 0) { handlerExport = esmExports @@ -39,24 +40,27 @@ const getMainExportFromCJS = (node: Statement) => { ] return handlerPaths.flatMap((handlerPath) => { - if (!isModuleExports(node, handlerPath) || node.expression.right.type !== 'CallExpression') { + if (!isModuleExports(node, handlerPath)) { return [] } - return getExportsFromCallExpression(node.expression.right) + return getExportsFromExpression(node.expression.right) }) } // Finds the main handler export in an ESM AST. -// eslint-disable-next-line complexity -const getMainExportFromESM = (node: Statement) => { +const getMainExportFromESM = (node: Statement, getAllBindings: BindingMethod) => { if (node.type !== 'ExportNamedDeclaration' || node.exportKind !== 'value') { return [] } - const { declaration } = node + const { declaration, specifiers } = node - if (!declaration || declaration.type !== 'VariableDeclaration') { + if (specifiers?.length > 0) { + return getExportsFromBindings(specifiers, getAllBindings) + } + + if (declaration?.type !== 'VariableDeclaration') { return [] } @@ -66,16 +70,46 @@ const getMainExportFromESM = (node: Statement) => { return type === 'VariableDeclarator' && id.type === 'Identifier' && id.name === 'handler' }) - if (handlerDeclaration?.init?.type !== 'CallExpression') { + const exports = getExportsFromExpression(handlerDeclaration?.init) + + return exports +} + +// Check if the Node is an ExportSpecifier that has a named export called `handler` +// either with Identifier `export { handler }` +// or with StringLiteral `export { x as "handler" }` +const isHandlerExport = (node: ExportNamedDeclaration['specifiers'][number]): node is ExportSpecifier => { + const { type, exported } = node + return ( + type === 'ExportSpecifier' && + ((exported.type === 'Identifier' && exported.name === 'handler') || + (exported.type === 'StringLiteral' && exported.value === 'handler')) + ) +} + +// Tries to resolve the export from a binding (variable) +// for example `let handler; handler = () => {}; export { handler }` would +// resolve correctly to the handler function +const getExportsFromBindings = (specifiers: ExportNamedDeclaration['specifiers'], getAllBindings: BindingMethod) => { + const specifier = specifiers.find(isHandlerExport) + + if (!specifier) { return [] } - const exports = getExportsFromCallExpression(handlerDeclaration.init) + const binding = getAllBindings().get(specifier.local.name) + const exports = getExportsFromExpression(binding) return exports } -const getExportsFromCallExpression = (node: CallExpression) => { +const getExportsFromExpression = (node: Expression | undefined | null) => { + // We're only interested in expressions representing function calls, because + // the ISC patterns we implement at the moment are all helper functions. + if (node?.type !== 'CallExpression') { + return [] + } + const { arguments: args, callee } = node if (callee.type !== 'Identifier') { diff --git a/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export.js b/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export.js new file mode 100644 index 000000000..33fd81b8c --- /dev/null +++ b/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export.js @@ -0,0 +1,7 @@ +import { schedule } from '@netlify/functions' + +const handler = schedule('@daily', async () => { + // function handler +}) + +export { handler } diff --git a/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed.js b/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed.js new file mode 100644 index 000000000..5b339ab39 --- /dev/null +++ b/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed.js @@ -0,0 +1,6 @@ +import { schedule } from '@netlify/functions' + +const _handler = schedule('@daily', async () => { + // function handler +}) +export { _handler as handler } diff --git a/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed_reassigned.js b/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed_reassigned.js new file mode 100644 index 000000000..eeba997d2 --- /dev/null +++ b/tests/fixtures/in-source-config/functions/cron_esm_no_direct_export_renamed_reassigned.js @@ -0,0 +1,8 @@ +import { schedule } from '@netlify/functions' + +const handler = async () => { + // function handler +} + +const _handler = schedule('@daily', handler) +export { _handler as handler } diff --git a/tests/fixtures/in-source-config/functions/cron_esm_schedule_reassigned.js b/tests/fixtures/in-source-config/functions/cron_esm_schedule_reassigned.js new file mode 100644 index 000000000..ec0a86396 --- /dev/null +++ b/tests/fixtures/in-source-config/functions/cron_esm_schedule_reassigned.js @@ -0,0 +1,7 @@ +import { schedule } from '@netlify/functions' + +const SCHEDULE = '@daily' + +export const handler = schedule(SCHEDULE, async () => { + // function handler +}) diff --git a/tests/main.js b/tests/main.js index bc0318028..c33f39ed5 100644 --- a/tests/main.js +++ b/tests/main.js @@ -2662,7 +2662,7 @@ testMany( 'Finds in-source config declarations using the `schedule` helper', ['bundler_default', 'bundler_esbuild', 'bundler_nft'], async (options, t) => { - const FUNCTIONS_COUNT = 7 + const FUNCTIONS_COUNT = 11 const { files } = await zipFixture(t, join('in-source-config', 'functions'), { opts: options, length: FUNCTIONS_COUNT, @@ -2692,7 +2692,7 @@ test('listFunctions includes in-source config declarations', async (t) => { const functions = await listFunctions(join(FIXTURES_DIR, 'in-source-config', 'functions'), { parseISC: true, }) - const FUNCTIONS_COUNT = 7 + const FUNCTIONS_COUNT = 11 t.is(functions.length, FUNCTIONS_COUNT) functions.forEach((func) => { t.is(func.schedule, '@daily')