-
Notifications
You must be signed in to change notification settings - Fork 8
[APPS][Connections Part 6] Build backend module graph during backend builds #353
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
Merged
gh-worker-dd-mergequeue-cf854d
merged 2 commits into
master
from
sdkennedy2/reachable-module-graph-call-sites
May 13, 2026
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
84 changes: 84 additions & 0 deletions
84
packages/plugins/apps/src/backend/ast-parsing/module-graph.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| // Copyright 2019-Present Datadog, Inc. | ||
|
|
||
| import { parseAst } from 'rollup/parseAst'; | ||
|
|
||
| import { createParsedModuleRecord } from './module-graph'; | ||
|
|
||
| const buildRoot = '/project'; | ||
|
|
||
| describe('Backend Functions - module graph records', () => { | ||
| test('Should create graph records for app-local backend modules', () => { | ||
| const record = createParsedModuleRecord( | ||
| '/project/src/backend/actions.backend.js', | ||
| buildRoot, | ||
| parseAst(` | ||
| import { getEcho } from './helpers/http.js'; | ||
| export function run() { | ||
| return getEcho(); | ||
| } | ||
| `), | ||
| ['/project/src/backend/helpers/http.js'], | ||
| ); | ||
|
|
||
| expect(record).toMatchObject({ | ||
| id: '/project/src/backend/actions.backend.js', | ||
| staticDependencies: ['/project/src/backend/helpers/http.js'], | ||
| unsupportedDependencies: [], | ||
| }); | ||
| expect(record?.ast.type).toBe('Program'); | ||
| }); | ||
|
|
||
| test.each([ | ||
| { description: 'package modules', id: '/project/node_modules/package/index.js' }, | ||
| { description: 'files outside buildRoot', id: '/external/helper.js' }, | ||
| { description: 'dist output', id: '/project/dist/helper.js' }, | ||
| { description: 'build output', id: '/project/build/helper.js' }, | ||
| { description: 'Vite cache output', id: '/project/.vite/helper.js' }, | ||
| { description: 'non-JavaScript files', id: '/project/src/backend/data.json' }, | ||
| ])('Should skip $description', ({ id }) => { | ||
| expect( | ||
| createParsedModuleRecord(id, buildRoot, parseAst('export const value = true;')), | ||
| ).toBeNull(); | ||
| }); | ||
|
|
||
| test.each([ | ||
| { | ||
| description: 'dynamic local imports', | ||
| code: "import('./helper.js');", | ||
| expected: { kind: 'dynamic-import', specifier: './helper.js' }, | ||
| }, | ||
| { | ||
| description: 'non-literal dynamic imports', | ||
| code: 'import(helperPath);', | ||
| expected: { kind: 'dynamic-import', specifier: 'non-literal dynamic import' }, | ||
| }, | ||
| { | ||
| description: 'local require calls', | ||
| code: "require('./helper.js');", | ||
| expected: { kind: 'require', specifier: './helper.js' }, | ||
| }, | ||
| ])('Should record unsupported $description', ({ code, expected }) => { | ||
| const record = createParsedModuleRecord( | ||
| '/project/src/backend/actions.backend.js', | ||
| buildRoot, | ||
| parseAst(code), | ||
| ); | ||
|
|
||
| expect(record?.unsupportedDependencies).toEqual([expected]); | ||
| }); | ||
|
|
||
| test('Should ignore package dynamic imports and require calls', () => { | ||
| const record = createParsedModuleRecord( | ||
| '/project/src/backend/actions.backend.js', | ||
| buildRoot, | ||
| parseAst(` | ||
| import('package'); | ||
| require('package'); | ||
| `), | ||
| ); | ||
|
|
||
| expect(record?.unsupportedDependencies).toEqual([]); | ||
| }); | ||
| }); |
184 changes: 184 additions & 0 deletions
184
packages/plugins/apps/src/backend/ast-parsing/module-graph.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| // Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. | ||
| // This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
| // Copyright 2019-Present Datadog, Inc. | ||
|
|
||
| import type { BaseNode, ImportExpression, Program, SimpleCallExpression } from 'estree'; | ||
| import path from 'path'; | ||
|
|
||
| import { ensureProgram, isStringLiteral } from './type-guards'; | ||
| import { walkAst } from './walk-ast'; | ||
|
|
||
| export interface ParsedModuleRecord { | ||
| id: string; | ||
| ast: Program; | ||
| staticDependencies: string[]; | ||
| unsupportedDependencies: ModuleDependency[]; | ||
| } | ||
|
|
||
| export interface ModuleDependency { | ||
| specifier: string; | ||
| kind: 'dynamic-import' | 'require'; | ||
| } | ||
|
|
||
| type ImportCallExpression = SimpleCallExpression & { callee: { type: 'Import' } }; | ||
|
|
||
| const DISALLOWED_GRAPH_DIRS = new Set(['node_modules', 'dist', 'build', '.vite']); | ||
| const PARSEABLE_FILE_RE = /\.(mjs|cjs|js|jsx|mts|cts|ts|tsx)$/; | ||
|
sdkennedy2 marked this conversation as resolved.
Outdated
|
||
|
|
||
| /** | ||
| * Creates the per-module analysis record consumed by backend-entry reachability | ||
| * analysis. The caller supplies canonical module IDs and already-resolved | ||
| * static dependency IDs instead of asking this module to resolve/load files. | ||
| * | ||
| * Returns null when the module is outside the analyzable app-local backend | ||
| * graph, allowing build collectors to skip package/generated modules without | ||
| * duplicating graph filtering rules. | ||
| */ | ||
| export function createParsedModuleRecord( | ||
| moduleId: string, | ||
| buildRoot: string, | ||
| ast: BaseNode, | ||
| staticDependencies: string[] = [], | ||
| ): ParsedModuleRecord | null { | ||
| if (!shouldTraverseCollectedModule(moduleId, buildRoot)) { | ||
| return null; | ||
| } | ||
|
|
||
| const program = ensureProgram(ast, moduleId); | ||
|
|
||
| return { | ||
| id: moduleId, | ||
| ast: program, | ||
| staticDependencies, | ||
| unsupportedDependencies: collectUnsupportedModuleDependencies(program), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Finds dependency forms that cannot be represented by the static dependency | ||
| * IDs supplied by the backend build collector. | ||
| */ | ||
| function collectUnsupportedModuleDependencies(ast: Program): ModuleDependency[] { | ||
| const dependencies: ModuleDependency[] = []; | ||
|
|
||
| walkAst(ast, dependencies, { | ||
| ImportExpression(node, { state }) { | ||
| const specifier = getImportExpressionSpecifier(node); | ||
| if (shouldFailDynamicImport(specifier)) { | ||
| state.push({ specifier, kind: 'dynamic-import' }); | ||
| } | ||
| }, | ||
| CallExpression(node, { state }) { | ||
| if (isImportCallExpression(node)) { | ||
| const specifier = getImportCallSpecifier(node); | ||
| if (shouldFailDynamicImport(specifier)) { | ||
| state.push({ specifier, kind: 'dynamic-import' }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| if (isLocalRequireCall(node)) { | ||
| state.push({ | ||
| specifier: getRequireSpecifier(node), | ||
| kind: 'require', | ||
| }); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| return dependencies; | ||
| } | ||
|
|
||
| /** | ||
| * Dynamic package imports are skipped, but local or non-literal dynamic imports | ||
| * could hide app-local action-catalog calls and must fail closed. | ||
| */ | ||
| function shouldFailDynamicImport(specifier: string): boolean { | ||
| return specifier === 'non-literal dynamic import' || isLocalSpecifier(specifier); | ||
| } | ||
|
|
||
| /** | ||
| * Keeps reachability traversal scoped to app-local JavaScript/TypeScript source | ||
| * modules that the backend build collector can safely analyze. | ||
| */ | ||
| export function shouldTraverseCollectedModule(moduleId: string, buildRoot: string): boolean { | ||
| if (!PARSEABLE_FILE_RE.test(moduleId)) { | ||
| return false; | ||
| } | ||
|
|
||
| const relativePath = path.relative(path.resolve(buildRoot), moduleId); | ||
| if (relativePath.startsWith('..') || path.isAbsolute(relativePath)) { | ||
| return false; | ||
| } | ||
|
|
||
| return !relativePath.split(path.sep).some((segment) => DISALLOWED_GRAPH_DIRS.has(segment)); | ||
| } | ||
|
|
||
| /** | ||
| * Reads the static string specifier from an ESTree dynamic import expression. | ||
| */ | ||
| function getImportExpressionSpecifier(node: ImportExpression): string { | ||
| return getLiteralSpecifier(node.source, 'non-literal dynamic import'); | ||
| } | ||
|
|
||
| /** | ||
| * Reads the static string specifier from Rollup's call-expression form for | ||
| * dynamic import. | ||
| */ | ||
| function getImportCallSpecifier(node: ImportCallExpression): string { | ||
| return getLiteralSpecifier(node.arguments[0], 'non-literal dynamic import'); | ||
| } | ||
|
|
||
| /** | ||
| * Reads the static string specifier from a CommonJS require call. | ||
| */ | ||
| function getRequireSpecifier(node: SimpleCallExpression): string { | ||
| return getLiteralSpecifier(node.arguments[0], 'local require'); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a literal string value when available, otherwise a diagnostic label | ||
| * used in fail-closed error messages. | ||
| */ | ||
| function getLiteralSpecifier(node: unknown, fallback: string): string { | ||
| if (isStringLiteral(node)) { | ||
| return node.value; | ||
| } | ||
| return fallback; | ||
| } | ||
|
|
||
| /** | ||
| * Narrows Rollup's dynamic import call-expression representation. | ||
| */ | ||
| function isImportCallExpression(node: SimpleCallExpression): node is ImportCallExpression { | ||
| return (node.callee as { type: string }).type === 'Import'; | ||
| } | ||
|
|
||
| /** | ||
| * Detects local CommonJS require calls. Package require calls are ignored | ||
| * because package modules are outside the app-local backend graph. | ||
| */ | ||
| function isLocalRequireCall(node: SimpleCallExpression): boolean { | ||
| if (node.callee.type !== 'Identifier' || node.callee.name !== 'require') { | ||
| return false; | ||
| } | ||
| const [source] = node.arguments; | ||
| return !source || !isStringLiteral(source) || isLocalSpecifier(source.value); | ||
| } | ||
|
|
||
| /** | ||
| * Returns whether an import specifier points at an app-local path. | ||
| */ | ||
| function isLocalSpecifier(specifier: string): boolean { | ||
| return specifier.startsWith('.') || specifier.startsWith('/'); | ||
| } | ||
|
|
||
| /** | ||
| * Builds the common fail-closed error for graph shapes that could hide an | ||
| * action-catalog connection ID. | ||
| */ | ||
| export function unsupportedModuleGraphDependency(filePath: string, unsupported: string): Error { | ||
| return new Error( | ||
| `Unsupported local module graph for ${filePath}: ${unsupported} could hide an action-catalog connectionId.`, | ||
| ); | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildseems potentially something a user could name a folder. We have that folder name inside one of our repos for example.Also should we include
.yarn? Any other folders that we might want? Should this be configurable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are good questions. A given module would need to be referenced via an import. This
DISALLOWED_GRAPH_DIRSis checked against each module path. Perhaps we should removedist,build,.vitesince they are unlikely to be referenced in an import statement and make sure we also cover yarn's package location as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, in that case, we may only need
node_modulesas any.yarnimports necessarily also includesnode_modulesinside the import path (after the .zip)