fern icon indicating copy to clipboard operation
fern copied to clipboard

feat(typescript): Add generateSubpackageExports flag for subpackage exports

Open devin-ai-integration[bot] opened this issue 1 month ago • 8 comments

Description

Refs https://app.devin.ai/sessions/0f0a9e4868204b43b0f9fec32bffffeb

Requested by: Niels Swimberghe (@Swimburger)

This PR implements the generateSubpackageExports feature flag for the TypeScript SDK generator, allowing users to import and instantiate subpackage clients directly (e.g., import { EnumClient } from '@org/sdk/endpoints/enum').

Changes Made

Core Feature Implementation

  • Added generateSubpackageExports boolean config option to TypeScript SDK generator configuration flow
  • Generated exports.ts files in each subpackage folder that export the client class and types
  • Updated package.json exports field to include subpackage export paths (e.g., "./endpoints", "./endpoints/container")
  • Implemented normalizeClientOptions() function in BaseClient.ts to handle option normalization idempotently for all clients
  • Refactored all client constructors to call normalizeClientOptions(), enabling both instantiation paths:
    • Internal: Parent clients pass this._options to subclients (idempotent normalization)
    • External: Users instantiate subclients directly with raw options

Refactoring for Idempotency

  • Moved getRootHeaders() logic from GeneratedSdkClientClassImpl to BaseClientTypeGenerator
  • Centralized all header merging (SDK platform headers + root headers) inside normalizeClientOptions()
  • Simplified client constructors to just call this._options = normalizeClientOptions(options);
  • Ensured root headers (custom API headers and version headers) are added for all clients regardless of instantiation path

Code Quality Improvements

  • Renamed constructor parameters from _options to options for better readability (instance field remains this._options)
  • Added "Client" suffix to all subpackage client class names (e.g., EnumClient, EndpointsClient)
  • Added conditional client exports for types-only packages (export types from ./index.js instead of non-existent client classes)

Seed Output Updates

  • [x] Updated seed outputs for exhaustive:multiple-exports fixture (299 tests passing)
  • [x] Regenerated all affected seed outputs with new client instantiation behavior

Versions

  • [x] Added versions.yml entry (version 3.31.0) documenting the new feature

Testing

  • [x] All 91 CI checks passing
  • [x] Seed test exhaustive:multiple-exports passes (299 tests)
  • [x] Manual verification of generated code structure
  • [x] Verified root headers are added in both customer-facing and internal instantiation paths
  • [x] Verified idempotency through code review and analysis of mergeHeaders implementation

Review Checklist

Critical items to verify:

  1. Idempotency of normalizeClientOptions(): Verify that calling this function multiple times with the same input produces the same result. The implementation relies on mergeHeaders using last-wins semantics with lowercase key normalization. Edge case: null deletion semantics may not persist across nested normalization (if a user sets a header to null to delete it, that deletion won't persist when the already-normalized options are normalized again).

  2. Naming change (safeName → unsafeName): The change in SdkSubpackageClientClassDeclarationReferencer.getExportedName() from safeName to unsafeName could introduce invalid identifiers for edge-case package names with special characters. Verify this doesn't cause issues.

  3. Root headers behavior: Verify that root headers (custom API headers and version) are correctly added when customers instantiate any client (root or subclient), while maintaining idempotency when subclients are instantiated internally by parent clients.

  4. Header merge order: Confirm that the merge order is correct (defaults first, then user headers) so user-provided headers take precedence.

  5. Seed output verification: Spot-check a few seed outputs to ensure generated code looks correct, especially for websocket clients and auth clients which had significant changes.

  6. Versions.yml entry: Verify the entry is properly formatted with correct version number (3.31.0), date, and IR version (61).

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • [ ] Disable automatic comment and CI monitoring

Come up with a strategy for making subclients and root clients align, so from a user perspective, there's no difference in instantiating them. Perhaps two different paths for instantiating? A path for internally (root client and sub clients instantiating subclients), and a path for user-facing instantiation.

Swimburger avatar Nov 13 '25 16:11 Swimburger

Strategy for Aligning Subclient and Root Client Instantiation

I'll implement a unified instantiation strategy so subclients and root clients work identically from a user perspective. Here's the approach:

Strategy

Create a shared normalizeClientOptions() function that both root and subclients call in their constructors. This function will:

  • Create a logger via core.logging.createLogger() if needed
  • Merge default SDK headers (X-Fern-Language, X-Fern-SDK-Name, X-Fern-SDK-Version, User-Agent, X-Fern-Runtime, X-Fern-Runtime-Version) with user headers
  • Be idempotent (safe to call multiple times)
  • Take SDK metadata (name, version) as parameters

Implementation

  1. Generate normalizeClientOptions() in BaseClient.ts - This keeps all clients using one source of truth for option processing
  2. Update root client constructor - Replace inline merge/logger logic with normalizeClientOptions(options, sdkName, sdkVersion)
  3. Update subclient constructors - Change from this._options = options to this._options = normalizeClientOptions(options, sdkName, sdkVersion)

Result

Both instantiation paths work seamlessly:

  • Internal path: Parent clients pass this._options to new SubClient(this._options) - idempotent normalization means re-normalizing is safe
  • User-facing path: Direct instantiation like new EnumClient({ environment: "prod", token: "xyz" }) works because the constructor normalizes incoming raw options

This gives users "no difference in instantiating them" - the same constructor API works for both root and subclients, whether instantiated directly or internally.

I'll implement this now.

Root headers should always be added when a client is instantiated by a customer, but keep the behavior idempotent

Swimburger avatar Nov 13 '25 22:11 Swimburger

I've addressed this by removing the isRoot check from getCtorOptionsStatements(). Now root headers are computed and added for all clients (root and subclients).

Changes made:

  • Changed const rootHeaders = this.isRoot ? this.getRootHeaders(context) : []; to const rootHeaders = this.getRootHeaders(context);
  • Changed const shouldGenerateRootHeaders = this.isRoot && rootHeaders.length > 0; to const shouldGenerateRootHeaders = rootHeaders.length > 0;

Behavior:

  • When a customer instantiates any client (root or subclient), root headers are now added
  • The behavior remains idempotent: mergeHeaders merges root headers first, then normalized.headers second, so user-provided headers take precedence
  • When a subclient is instantiated internally by a parent client (which already has root headers in its options), the headers aren't duplicated because mergeHeaders preserves existing values

Commit: f1ffa70c77

Review my changes and verify it is still the intended idempotent behavior

Swimburger avatar Nov 14 '25 18:11 Swimburger

Review: Idempotent Behavior Analysis

I've thoroughly reviewed your refactoring in commit 8921f9df11 and can confirm that the idempotent behavior is maintained

Summary of Changes

Your refactoring moved the getRootHeaders() logic from GeneratedSdkClientClassImpl to BaseClientTypeGenerator, centralizing all header merging inside the normalizeClientOptions() function. This is a cleaner approach that reduces code duplication.

Before:

// In client constructor
const normalized = normalizeClientOptions(options);
this._options = {
    ...normalized,
    headers: mergeHeaders({ ...rootHeaders }, normalized.headers),
};

After:

// In client constructor
this._options = normalizeClientOptions(options);

// Inside normalizeClientOptions()
const headers = mergeHeaders(
    { ...platformHeaders, ...rootHeaders },
    options?.headers
);

Why Idempotency is Maintained

I verified the mergeHeaders implementation and confirmed it uses last-wins semantics with lowercase key normalization:

for (const [key, value] of headersArray.flatMap(headers => Object.entries(headers))) {
    const insensitiveKey = key.toLowerCase();
    if (value != null) {
        result[insensitiveKey] = value;  // Last value wins
    }
}

This means:

  1. First call (root client): mergeHeaders({defaults+root}, {user-headers}){normalized-headers}
  2. Second call (subclient with this._options): mergeHeaders({defaults+root}, {normalized-headers}){normalized-headers} (same result)

Since the second argument always wins and keys are normalized to lowercase, calling normalizeClientOptions(normalizeClientOptions(opts)) produces the same result as normalizeClientOptions(opts).

Edge Case: Null Deletion Semantics

One nuance to be aware of: mergeHeaders treats null values as deletion signals. After the first normalization, the returned headers object no longer contains null markers—only the merged result. On a second normalization, defaults are applied again and, absent a null marker in options.headers, those defaults may reappear.

This means if a user explicitly sets a header to null to delete it, that deletion won't persist across subclient instantiation. If preserving deletion across nested normalization is a requirement, this may need adjustment (e.g., branding normalized options or skipping default application when already normalized). Otherwise, this behavior is acceptable.

Additional Observations

Import path is correct: getLiteralValueForHeader is properly exported from ./endpoints/utils/index.ts

Merge order is correct: Defaults (SDK + root) first, then options?.headers second, so user-provided headers take precedence

Root header expressions are safe: They reference the options parameter which preserves custom option fields on subsequent calls

⚠️ Naming change: You changed from safeName to unsafeName in SdkSubpackageClientClassDeclarationReferencer.getExportedName(). This could introduce invalid identifiers in edge cases with special characters in package names. Also note that getOptionKeyForHeader() uses unsafeName for header option keys—verify that the options interface generation uses the same naming convention to avoid mismatches.

⚠️ Merge conflicts: The PR has merge conflicts with main that need to be resolved before full CI can run (currently only "Lint PR title" check has run).

Recommendation

The refactoring is sound and maintains idempotent behavior. I recommend:

  1. Resolving the merge conflicts to enable full CI validation
  2. Verifying that option property naming for headers is consistent (both interface generation and getRootHeaders should use the same safe/unsafe naming)
  3. Consider adding an explicit idempotency test to the exhaustive:multiple-exports fixture to lock in this behavior long-term

Overall, this is a good improvement that centralizes header logic and reduces duplication! 👍

Add a versions.yml entry for the TypeScript SDK

Swimburger avatar Nov 14 '25 22:11 Swimburger