Skip to content

feat: make cloud provider packages optional#298

Merged
tianzhou merged 5 commits intomainfrom
fix/optional-cloud-packages
Apr 2, 2026
Merged

feat: make cloud provider packages optional#298
tianzhou merged 5 commits intomainfrom
fix/optional-cloud-packages

Conversation

@tianzhou
Copy link
Copy Markdown
Member

@tianzhou tianzhou commented Apr 2, 2026

Summary

  • Move @aws-sdk/rds-signer and @azure/identity from dependencies to optionalDependencies
  • Switch static imports to dynamic import() so packages are only loaded when AWS IAM or Azure AD auth is used
  • Add both to tsup external list to prevent bundling CJS into ESM (same pattern as database drivers in Fix: externalize database drivers to prevent dynamic require error #291)

Closes #295

Test plan

  • pnpm run build:backend succeeds
  • pnpm test:build smoke test passes
  • pnpm test:unit passes (including aws-rds-signer tests)
  • Verified smoke test catches missing externals for database drivers (cloud packages are ESM-native so don't trigger CJS errors, but externalization still prevents bundling ~1MB of SDK code)

🤖 Generated with Claude Code

Move @aws-sdk/rds-signer and @azure/identity from dependencies to
optionalDependencies and switch to dynamic import() so they are only
loaded when AWS IAM or Azure AD auth is actually used. Also add both
to tsup external list to prevent bundling.

Closes #295

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 2, 2026 15:45
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes AWS/Azure cloud auth SDKs optional at runtime by moving them to optionalDependencies, switching to dynamic import() so they’re only loaded when needed, and ensuring the bundler doesn’t inline them into the ESM build.

Changes:

  • Move @aws-sdk/rds-signer and @azure/identity from dependencies to optionalDependencies.
  • Replace static imports with dynamic import() in the AWS RDS signer helper and SQL Server Azure AD auth path.
  • Add both packages to tsup’s external list to avoid bundling them.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
tsup.config.ts Externalizes cloud auth SDK packages in the backend bundle.
src/utils/aws-rds-signer.ts Lazily loads @aws-sdk/rds-signer at call time.
src/connectors/sqlserver/index.ts Lazily loads @azure/identity only when Azure AD auth is selected.
package.json Makes cloud auth SDKs optional dependencies.

…cloud packages

- Update tsup external comment to mention cloud auth packages
- Catch import failures for @aws-sdk/rds-signer and @azure/identity
  and rethrow with actionable install instructions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

src/connectors/sqlserver/index.ts:110

  • DefaultAzureCredential.getToken() can return null when no token could be acquired. Accessing token.token will then throw a non-obvious runtime error. Handle the null case explicitly and throw a clearer error (or include context about Azure credential resolution).
            }
            const credential = new DefaultAzureCredential();
            const token = await credential.getToken("https://database.windows.net/");
            config.authentication = {
              type: "azure-active-directory-access-token",
              options: {
                token: token.token,

…ackages

- Only translate ERR_MODULE_NOT_FOUND into install hint, rethrow other
  errors so real SDK failures are not masked (reuses isDriverNotInstalled)
- Use pnpm-appropriate install command in error messages
- Refine tsup comment to distinguish CJS driver externals from cloud
  auth bundle-size externals
- Add test verifying isDriverNotInstalled works with scoped package names

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/connectors/sqlserver/index.ts:120

  • The missing-package error thrown in the inner catch (importError) is immediately caught by the outer catch and wrapped as Failed to get Azure AD token: .... When @azure/identity is missing this prefix is misleading (it’s not a token failure). Consider letting the actionable install error bubble up unchanged (e.g., rethrow without wrapping when it’s already an install/missing-dep error).
            } catch (importError) {
              if (isDriverNotInstalled(importError, "@azure/identity")) {
                throw new Error(
                  'Azure AD authentication requires the "@azure/identity" package. Install it with: pnpm add @azure/identity'
                );
              }
              throw importError;
            }
            const credential = new DefaultAzureCredential();
            const token = await credential.getToken("https://database.windows.net/");
            config.authentication = {
              type: "azure-active-directory-access-token",
              options: {
                token: token.token,
              },
            };
          } catch (error: unknown) {
            const errorMessage = error instanceof Error ? error.message : String(error);
            throw new Error(`Failed to get Azure AD token: ${errorMessage}`);
          }

Comment on lines +1 to +5
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { isDriverNotInstalled } from '../module-loader.js';

describe('generateRdsAuthToken missing package', () => {
it('should throw actionable error when @aws-sdk/rds-signer is not installed', async () => {
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file imports vi and beforeEach but never uses them, and the first it(...) is marked async without awaiting anything. Clean up unused imports / remove the unnecessary async to keep the test suite lint/tsc-friendly.

Suggested change
import { describe, it, expect, vi, beforeEach } from 'vitest';
import { isDriverNotInstalled } from '../module-loader.js';
describe('generateRdsAuthToken missing package', () => {
it('should throw actionable error when @aws-sdk/rds-signer is not installed', async () => {
import { describe, it, expect } from 'vitest';
import { isDriverNotInstalled } from '../module-loader.js';
describe('generateRdsAuthToken missing package', () => {
it('should throw actionable error when @aws-sdk/rds-signer is not installed', () => {

Copilot uses AI. Check for mistakes.
Comment on lines +4 to +13
describe('generateRdsAuthToken missing package', () => {
it('should throw actionable error when @aws-sdk/rds-signer is not installed', async () => {
// Simulate ERR_MODULE_NOT_FOUND for @aws-sdk/rds-signer
const err = new Error(
"Cannot find package '@aws-sdk/rds-signer' imported from /fake/path"
);
(err as NodeJS.ErrnoException).code = 'ERR_MODULE_NOT_FOUND';

expect(isDriverNotInstalled(err, '@aws-sdk/rds-signer')).toBe(true);
});
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suite name suggests it’s validating generateRdsAuthToken behavior when @aws-sdk/rds-signer is missing, but the assertions only test isDriverNotInstalled(). This leaves the new user-facing error path in generateRdsAuthToken() untested. Consider either renaming the suite to match what’s tested or adding a test that forces the dynamic import("@aws-sdk/rds-signer") to fail and asserts the thrown install message.

Copilot uses AI. Check for mistakes.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Move the dynamic import of @azure/identity outside the token-fetching
try/catch so the install-instruction error propagates directly instead
of being wrapped as "Failed to get Azure AD token: ..."

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@tianzhou tianzhou merged commit 4268859 into main Apr 2, 2026
6 checks passed
amansingh63 added a commit to amansingh63/dbhub-analytics that referenced this pull request Apr 2, 2026
Upstream changes:
- Make @aws-sdk/rds-signer and @azure/identity optional (bytebase#298)
- Fix PostgreSQL view comments in getTableComment() (bytebase#297)
- Version bump 0.21.0 → 0.21.1

Conflicts resolved: README.md, package.json, server.json, tsup.config.ts
(kept dbhub-analytics branding, merged upstream code changes)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tianzhou added a commit that referenced this pull request Apr 3, 2026
Lockfile was not regenerated when @aws-sdk/rds-signer and @azure/identity
were moved to optionalDependencies in #298, causing CI frozen-lockfile failures.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support optional cloud provider packages?

2 participants