[APPS][Connections Part 4] Analyze backend connection IDs across modules#341
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd725d1bd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isActionCatalogSource(node.source.value)) { | ||
| importBindings.set(spec.local.name, { | ||
| source: node.source.value, | ||
| imported: 'default', | ||
| }); |
There was a problem hiding this comment.
Handle default-exported connection constants
When a backend uses a default-imported connection ID, e.g. import HTTP_ID from './connections' with export default 'conn', this records the binding as imported name default, but resolveExportedExpression only resolves top-level consts and ExportNamedDeclarations, not ExportDefaultDeclaration, so extraction rejects the build with export 'default' not found. Either avoid marking default imports as resolvable or add default-export handling so this supported-looking import form works.
Useful? React with 👍 / 👎.
| mod, | ||
| graph, | ||
| resolutionStack, | ||
| ); |
There was a problem hiding this comment.
Detect cyclic object-member aliases
For a member expression whose const initializer is another identifier, this recurses with the same resolutionStack, so a cyclic alias such as const A = B; const B = A; request({ connectionId: A.HTTP }) never hits the existing cycle/depth checks and overflows the stack instead of producing a controlled unsupported-pattern error. Add the object name to the stack before following identifier aliases, similar to identifier resolution.
Useful? React with 👍 / 👎.
8000ecd to
b3ebcdc
Compare
fd725d1 to
e512609
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e512609b03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import path from 'path'; | ||
| import type { AstNode, PluginContext } from 'rollup'; | ||
| import { transformWithEsbuild } from 'vite'; | ||
|
|
There was a problem hiding this comment.
Match generated segments relative to the build root
When context.buildRoot itself contains a build path segment (for example the existing Vite test fixture uses /build, and CI checkouts commonly do), every resolved source file such as /build/src/helper.ts matches this regex. shouldTraverseResolvedId then drops those helper modules before analyzing them, so connection IDs in imported modules are omitted from the manifest solely because of the checkout directory; apply the generated-output check to the path relative to buildRoot instead.
Useful? React with 👍 / 👎.
| for (const node of target.ast.body) { | ||
| if (node.type === 'ExportNamedDeclaration') { |
There was a problem hiding this comment.
Handle default-exported connection IDs
When a backend imports a static ID with default syntax, e.g. import HTTP_ID from './connections' where connections.ts has export default 'conn' or export default HTTP_ID, collectModuleBindings records the import as imported: 'default' but this resolver only scans named exports and export-star declarations before throwing export 'default' not found. That rejects valid, statically analyzable module graphs that use default exports for shared connection constants.
Useful? React with 👍 / 👎.
b3ebcdc to
a367b98
Compare
e512609 to
36d9c01
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36d9c01133
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| importBindings.set(spec.local.name, { | ||
| source: node.source.value, | ||
| imported: 'default', | ||
| }); |
There was a problem hiding this comment.
Handle default-exported connection IDs
When a backend uses a default import for a connection ID, e.g. import HTTP_ID from './connections'; request({ connectionId: HTTP_ID }), this records the binding as the imported name default, but resolveExportedExpression() only searches named consts/re-exports and never handles ExportDefaultDeclaration. The graph analyzer therefore throws export 'default' not found for valid modules such as export default 'conn' or const HTTP_ID = 'conn'; export default HTTP_ID, so default-imported constants are unusable despite being added to the resolvable import map here.
Useful? React with 👍 / 👎.
| for (const arg of call.arguments) { | ||
| if (arg.type === 'Identifier' && imports.functions.has(arg.name)) { | ||
| fail( | ||
| `Unsupported action-catalog call in ${filePath}: higher-order action-catalog invocation cannot be statically analyzed for connectionId.`, | ||
| ); |
There was a problem hiding this comment.
Respect local shadowing before rejecting higher-order calls
This rejects any call that passes an identifier with the same name as an action-catalog import, even when that identifier is a local parameter or block binding rather than the imported action. For example, a module that imports request for one backend call and also has function log(request) { console.log(request); } will now fail with higher-order action-catalog invocation, although no action-catalog function is being passed. The check needs scope awareness or should avoid treating shadowed identifiers as the imported function.
Useful? React with 👍 / 👎.
a367b98 to
a735063
Compare
36d9c01 to
dc07d6b
Compare
a735063 to
397f9ba
Compare
dc07d6b to
1d1861d
Compare
1d1861d to
cda1903
Compare
1b589d7 to
e56eae5
Compare
cda1903 to
88e0756
Compare
e56eae5 to
e8fff5b
Compare
88e0756 to
0a7612f
Compare
e8fff5b to
e142b33
Compare
1df5a0b to
b94a0f8
Compare
e142b33 to
883e8e4
Compare
b94a0f8 to
ed0d7a7
Compare
638ffe0 to
705b4da
Compare
ed0d7a7 to
057448e
Compare
705b4da to
c3a5e2c
Compare
057448e to
82f45a2
Compare

Motivation
This is part 4 of the connection ID manifest stack and brings the split stack up to the intended behavior from draft PR #330. Real backend functions often delegate action-catalog calls and connection ID definitions to helper modules, barrels, or shared connection modules.
Changes
Adds bounded local module graph analysis starting from each backend entry module. The extractor traverses static local imports and re-exports inside
buildRoot, skips package/virtual/generated/out-of-root modules, detects action-catalog calls in reachable helper modules, and resolves imported connection constants and object members through named exports, aliases, local import/export relays, andexport *barrels.Example now supported:
Module loading uses the Rollup/Vite context (
resolve,load,parse) and includes a targeted fallback for Vite's unsupportedModuleInfo.codecase by reading the resolved file and transforming TS/TSX/JSX through esbuild before parsing.The analyzer remains fail-closed for dynamic local imports, local
require, unresolved local imports required for analysis, mutable imported bindings, unresolved imported values, re-export cycles, action aliases, namespace destructuring, higher-order action invocation, and optional action calls.QA Instructions
NODE_OPTIONS=--experimental-vm-modules ~/.yarn/switch/bin/yarn workspace @dd/tests jest --config jest.config.ts ../plugins/apps/src/backend/discovery.test.ts ../plugins/apps/src/backend/extract-connection-ids.test.ts ../plugins/apps/src/index.test.ts ../plugins/apps/src/vite/index.test.ts ../plugins/apps/src/vite/dev-server.test.ts ../plugins/apps/src/upload.test.ts --runInBand.~/.yarn/switch/bin/yarn workspace @dd/apps-plugin typecheck.~/.yarn/switch/bin/yarn workspace @dd/tests typecheck.~/.yarn/switch/bin/yarn workspace @dd/tests test:e2e appsPlugin/appsPlugin.spec.ts --project="chrome | vite".Blast Radius
This affects apps-plugin backend function transform analysis and may fail builds for backend modules that use unsupported connectionId/action-catalog patterns. The behavior is intentionally conservative and limited to local modules under the app build root.
Documentation