fix(ai-bedrock): use string-literal specifiers for AWS SDK dynamic imports#958
fix(ai-bedrock): use string-literal specifiers for AWS SDK dynamic imports#958feiiiiii5 wants to merge 2 commits into
Conversation
…ports (TanStack#929) The adapter loaded its AWS SDK dependencies through dynamic imports whose specifier was a variable — `const mod = '@aws-sdk/...'; import(/* @vite-ignore */ mod)`. Static bundlers (esbuild, bun build, Rollup) cannot resolve import() when the argument is not a string literal, so the SDK was left external and runtime resolution failed in node_modules-free server bundles. The reported failure: a Bun build of a Bedrock-backed server threw "Cannot find package @aws-sdk/client-bedrock-runtime" on the first request. Switch both anti-pattern sites (auth.ts credential-providers import and converse-text.ts client-bedrock-runtime import) to string-literal specifiers. The dynamic import still defers the Node-only SDK until first use, so the module-load graph is unchanged, but bundlers can now statically resolve and include the SDK in self-contained server artifacts. Adds a source-level regression test that pins the literal-specifier contract for both sites — if a future refactor brings the variable-specifier pattern back, the test fails before the regression ships. Fixes TanStack#929.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAWS SDK dynamic imports in the Bedrock adapter now use string-literal specifiers for static bundler resolution. A Vitest source-analysis regression test enforces this pattern, and a patch changeset documents the fix. ChangesBedrock bundler import handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/ai-bedrock/src/adapters/converse-text.ts (1)
111-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the return type with the JSDoc comment.
The comment on line 100 mentions that
typeof import(...)is used to maintain full typing without a cast, but the method returnsPromise<typeof BedrockRuntime>. You can inline thetypeof import(...)here to match the comment perfectly, which may also remove the need for a separate top-level type import forBedrockRuntime.💅 Proposed refactor
- protected importBedrockRuntime(): Promise<typeof BedrockRuntime> { + protected importBedrockRuntime(): Promise<typeof import('`@aws-sdk/client-bedrock-runtime`')> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-bedrock/src/adapters/converse-text.ts` at line 111, Update the return type of importBedrockRuntime to inline the typeof import(...) expression described by its JSDoc, replacing the separate BedrockRuntime type reference while preserving the method’s Promise-based typing and implementation behavior.packages/ai-bedrock/tests/bundler-static-analysis.test.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder imports to place built-in Node modules before external packages.
As indicated by ESLint, built-in modules (
node:fs,node:url) should precede external dependencies (vitest).💅 Proposed refactor
-import { describe, expect, it } from 'vitest' -import { readFileSync } from 'node:fs' -import { fileURLToPath } from 'node:url' +import { readFileSync } from 'node:fs' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-bedrock/tests/bundler-static-analysis.test.ts` around lines 1 - 3, Reorder the imports in bundler-static-analysis.test.ts so the built-in Node modules, including node:fs and node:url, appear before the external vitest import, without changing any import contents or test behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/ai-bedrock/src/adapters/converse-text.ts`:
- Line 111: Update the return type of importBedrockRuntime to inline the typeof
import(...) expression described by its JSDoc, replacing the separate
BedrockRuntime type reference while preserving the method’s Promise-based typing
and implementation behavior.
In `@packages/ai-bedrock/tests/bundler-static-analysis.test.ts`:
- Around line 1-3: Reorder the imports in bundler-static-analysis.test.ts so the
built-in Node modules, including node:fs and node:url, appear before the
external vitest import, without changing any import contents or test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad9645a9-3c77-45d2-a0db-dc82ef68c21f
📒 Files selected for processing (4)
.changeset/ai-bedrock-static-bundler-specifiers.mdpackages/ai-bedrock/src/adapters/converse-text.tspackages/ai-bedrock/src/utils/auth.tspackages/ai-bedrock/tests/bundler-static-analysis.test.ts
- Reorder imports in `bundler-static-analysis.test.ts` so built-in `node:fs`
and `node:url` precede the external `vitest` import (built-in-first per
@typescript-eslint/consistent-type-imports and the repo's other tests).
- Align the `importBedrockRuntime` JSDoc with the actual return type
(`typeof BedrockRuntime` from the type-only namespace import, not
`typeof import(...)` — the inline form is rejected by the repo's
`consistent-type-imports` config, so the namespace import stays).
CodeRabbit's first nitpick (inline `typeof import('@aws-sdk/...')` on the
return type) was tested and reverted: it triggers
`@typescript-eslint/consistent-type-imports`'s `import()` type-annotation
ban, so the type-only namespace import is preserved as-is.
|
@coderabbitai thanks for the review. Both nitpicks addressed in 1. 2. Local pre-PR gate re-run after the refactor: |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
🎯 Changes
@tanstack/ai-bedrockloaded its AWS SDK dependencies (@aws-sdk/client-bedrock-runtimeand@aws-sdk/credential-providers) through dynamic imports whose specifier was a variable:```ts
const mod = '@aws-sdk/client-bedrock-runtime'
return import(/* @vite-ignore */ mod)
```
Static bundlers — esbuild, bun build, Rollup — cannot resolve `import()` when the argument is not a string literal, so the SDK was left external and runtime resolution failed in `node_modules`-free server bundles. The reported failure in #929: a Bun build of a Bedrock-backed server threw `Cannot find package @aws-sdk/client-bedrock-runtime` on the first request.
This PR switches both anti-pattern sites to string-literal specifiers:
The dynamic import still defers the Node-only SDK until first use, so the module-load graph is unchanged — but bundlers can now statically resolve and include the SDK in self-contained server artifacts.
Also adds a source-level regression test (`packages/ai-bedrock/tests/bundler-static-analysis.test.ts`) that pins the literal-specifier contract for both sites — if a future refactor brings the variable-specifier pattern back, the test fails before the regression ships. A source-level regex check was chosen over a real bundler run because the contract is purely about static analysability; a regex over the source is the smallest test that faithfully pins it, with no new dev dependency (esbuild/acorn/etc.) and no slow build step in the unit suite.
Vite's dev-time `optimizeDeps` pre-bundler may try to scan the now-static SDK import for the browser and fail on the SDK's Node-only exports. Browser-side use of this server-only adapter is unsupported; the comments in `auth.ts` and `converse-text.ts` document the `optimizeDeps.exclude` workaround and point to the matching pattern already in `examples/ts-react-chat/vite.config.ts`.
✅ Checklist
🚀 Release Impact
Test Plan
Ran locally (skipping `test:react-native` which needs a RN simulator — RN packages are not touched by this PR):
```
pnpm exec nx affected --targets=test:sherif,test:knip,test:docs,test:kiira,test:eslint,test:lib,test:types,test:build,build --exclude=examples/,testing/
pnpm test:dts
```
All green (12/12 Nx tasks, `test:dts` clean — 127 .d.ts files, no dangling references). The new `bundler-static-analysis.test.ts` passes 4/4. The `@tanstack/ai-bedrock:test:eslint` initially flagged a redundant `as Promise` cast that the string-literal specifier made unnecessary; that cast has been removed.
Fixes #929.
Summary by CodeRabbit
Bug Fixes
Tests