graphql-request icon indicating copy to clipboard operation
graphql-request copied to clipboard

Rewrite Upload extension to use schema-driven variable traversal

Open jasonkuhrt opened this issue 2 months ago • 1 comments

Problem

The Upload extension currently uses runtime type checking (instanceof Blob) to detect and extract files from variables. This has several limitations:

  1. Inefficient: Blindly traverses entire variable structure regardless of schema
  2. Incomplete detection: Only checks top-level variables (would miss { input: { avatar: blob } })
  3. Inconsistent: Doesn't follow the same pattern as custom scalar encoding
  4. No schema awareness: Doesn't leverage GraphQL type information

Current Implementation

Upload.ts:66-69 - Shallow detection:

const isUploadRequest = (request: RequestAnalyzedInput) => {
  if (!request.variables) return false
  return Object.values(request.variables).some(_ => _ instanceof Blob)
}

extractFiles.ts - Traverses everything:

  • Recursively visits every array, object, and value
  • No schema guidance on where Upload scalars are located
  • Performance: O(all nodes in variables)

Existing Pattern: Custom Scalar Encoding

Graffle already has schema-driven variable traversal for custom scalar encoding!

src/requestPipeline/encode.ts:7-87 demonstrates the pattern:

  • Accesses SDDM (Schema-Driven Data Map) from input.state.configuration.schema.current.map
  • Walks variable definitions from the GraphQL operation
  • Looks up types in SDDM
  • Visits only fields containing custom scalars (sddmNode.fcs)
  • Performance: O(custom scalar paths only)

This is exactly what Upload needs, but for Upload scalar types instead of all custom scalars.

Proposed Solution: Simple API

Ideal Extension Author API

Extension.create('Upload')
  .requestInterceptor(async ({ pack }) => {
    // Get all variables of type 'Upload'
    const uploadFiles = visitVariableTypes(pack.input, ['Upload'])
    // Returns: Map<Blob, string[]> - each Upload value and its paths
    
    if (uploadFiles.size === 0) return pack()
    
    return pack({
      using: { 
        body: () => createMultipartBody(uploadFiles, pack.input.request) 
      },
      input: {
        ...pack.input,
        transport: {
          ...pack.input.transport,
          headers: { 'content-type': '' }
        }
      }
    })
  })

Implementation Signature

/**
 * Extract all variables matching specified GraphQL type names.
 * Uses schema information (SDDM) to efficiently visit only relevant paths.
 * 
 * @param packInput - Request pipeline pack input containing request, state, variables
 * @param typeNames - GraphQL type names to match (e.g., ['Upload', 'File'])
 * @returns Map of values to their paths within variables (e.g., "input.avatar", "attachments.0")
 * 
 * @example
 * const uploadFiles = visitVariableTypes(pack.input, ['Upload'])
 * // Map { <Blob> => ["variables.input.avatar", "variables.documents.0"] }
 */
export const visitVariableTypes = (
  packInput: {
    request: Grafaid.RequestAnalyzedInput
    state: Context
  },
  typeNames: string[]
): Map<any, string[]>

Key Design Principles

  1. Simple - Extension authors just specify type names
  2. Schema-driven - Leverages SDDM internally (hidden complexity)
  3. Efficient - Only visits paths that match specified types
  4. Consistent - Same pattern as encodeRequestVariables
  5. Type-safe - Returns clean Map structure

Implementation Plan

1. Create Shared Utility Module

Location: src/requestPipeline/visitVariableTypes.ts

Based on encodeRequestVariables pattern but generalized:

  • Extract SDDM from state.configuration.schema.current.map
  • Get variable definitions from request.operation.variableDefinitions
  • For each variable, look up its type in SDDM
  • Recursively visit structure, checking if types match typeNames
  • Collect matching values and their paths
  • Return Map<any, string[]>

2. Export from Extension API

Location: src/exports/extension_exports.ts

export { visitVariableTypes } from '../requestPipeline/visitVariableTypes.js'

3. Rewrite Upload Extension

Location: src/extensions/Upload/Upload.ts

  • Replace runtime instanceof Blob detection with schema-driven visitVariableTypes
  • Simplify extractFiles.ts or remove it entirely (logic moves to visitVariableTypes)
  • Update createBody.ts to work with Map returned by visitVariableTypes

4. Update Tests

Location: src/extensions/Upload/Upload.test.ts

  • Add tests for nested Upload scalars (currently likely untested)
  • Add tests for multiple Upload scalars in same request
  • Add tests for schema without Upload scalar (graceful fallback)

Benefits

Aspect Current (runtime check) Proposed (schema-driven)
Performance O(all nodes) O(Upload paths only)
Accuracy Shallow, may miss nested Complete, schema-guided
Consistency Different from encoding Same pattern as encoding
Schema required No Yes (but already available)
Type safety Runtime only Schema contract enforced

Additional Context

SDDM Structure Reference

Extensions using visitVariableTypes don't need to understand SDDM internals, but for implementation:

  • sddm.types[typeName] - Look up type by name
  • InputObject.fcs - Field names containing custom scalars
  • InputObject.f - Field definitions
  • ArgumentOrInputField.nt - Named type reference
  • ArgumentOrInputField.it - Inline type (nullability/list info)

Type Guards (Already Exist)

From src/docpar/core/sddm/SchemaDrivenDataMap.ts:253-291:

  • isEnum()
  • isCustomScalarName()
  • isScalar()
  • isInputObject()
  • isOutputField()

These can be used internally by visitVariableTypes.

Related Code

  • Encoding pattern: src/requestPipeline/encode.ts:7-87
  • SDDM types: src/docpar/core/sddm/SchemaDrivenDataMap.ts
  • Current Upload: src/extensions/Upload/Upload.ts
  • Current extraction: src/extensions/Upload/extractFiles.ts
  • Extension builder: src/context/fragments/extensions/dataType/builder.ts

Acceptance Criteria

  • [ ] visitVariableTypes utility implemented and exported
  • [ ] Upload extension rewritten to use visitVariableTypes
  • [ ] Tests pass including new nested Upload scalar tests
  • [ ] Performance improvement measurable for large variable structures
  • [ ] Documentation updated with extension API example
  • [ ] No breaking changes to Upload extension public API

Future Extensions

This pattern enables other extensions that need type-aware variable processing:

  • File validation extension (check file size/type based on schema directives)
  • Variable encryption extension (encrypt specific scalar types)
  • Logging extension (redact sensitive scalar types)

Priority: Medium Labels: enhancement, extension-system, upload Estimated Effort: 1-2 days

jasonkuhrt avatar Oct 24 '25 14:10 jasonkuhrt

Tree-Shaking Strategy: Export Conditions

Problem

While implementing schema-driven variable traversal (using mapVariablesByTypeNames), we identified a bundle size concern:

SDDM can be large (~10-50KB depending on schema size). Manual clients without SDDM shouldn't be forced to bundle the SDDM-dependent code path.

Solution: Export Conditions

Use Node.js export conditions to provide two versions of the Upload extension:

  1. Runtime version (default): Uses instanceof Blob checks, no SDDM required
  2. SDDM-optimized version: Uses mapVariablesByTypeNames for schema-guided traversal

Users opt-in via bundler configuration to use the optimized version.


Implementation

Package Structure

src/extensions/Upload/
├── Upload.ts              # Runtime version (default export)
├── Upload-with-sddm.ts    # SDDM-optimized version
└── helpers.ts             # Shared logic

Package Exports

{
  "exports": {
    "./extensions/upload": {
      "graffle-sddm": "./build/extensions/Upload/Upload-with-sddm.js",
      "default": "./build/extensions/Upload/Upload.js"
    }
  }
}

Code Pattern

Both files use literal boolean constants for dead code elimination:

// Upload.ts (runtime version)
const __HAS_SDDM__ = false  // ← Literal false

export const Upload = Extension.create('Upload')
  .requestInterceptor(async ({ pack }) => {
    if (__HAS_SDDM__) {
      // SDDM path - ELIMINATED by bundler
      throw new Error('Unreachable')
    } else {
      // Runtime path - KEPT by bundler
      return useExtractFiles(pack)
    }
  })

// Upload-with-sddm.ts (optimized version)
const __HAS_SDDM__ = true  // ← Literal true

export const Upload = Extension.create('Upload')
  .requestInterceptor(async ({ pack }) => {
    if (__HAS_SDDM__) {
      // SDDM path - KEPT by bundler
      const sddm = pack.input.state.configuration.schema.current.map!
      return useMapVariablesByTypeNames(sddm, pack)
    } else {
      // Runtime path - ELIMINATED by bundler
      throw new Error('Unreachable')
    }
  })

Bundlers see if (true) or if (false) and eliminate the dead branch entirely!


User Configuration

Vite

// vite.config.js
export default {
  resolve: {
    conditions: ['graffle-sddm']
  }
}

Webpack

// webpack.config.js
module.exports = {
  resolve: {
    conditionNames: ['graffle-sddm', 'import', 'require']
  }
}

Rollup

// rollup.config.js
import resolve from '@rollup/plugin-node-resolve'

export default {
  plugins: [
    resolve({
      exportConditions: ['graffle-sddm']
    })
  ]
}

esbuild

esbuild.build({
  conditions: ['graffle-sddm']
})

User Code

// Same import in both cases
import { Upload } from 'graffle/extensions/upload'

// Without config: resolves to Upload.ts (runtime)
// With config: resolves to Upload-with-sddm.ts (optimized)

Works for both generated AND manual clients!


Benefits

Perfect tree-shaking: Literal constants + export conditions = guaranteed dead code elimination ✅ User opt-in: One-time bundler config ✅ Zero code changes: Same import path works for both modes ✅ Standard feature: Export conditions are official Node.js feature (supported by all modern bundlers) ✅ No duplication in source: Most logic in shared helpers.ts, only interceptor wrapper duplicated (~20 lines) ✅ Works everywhere: Generated clients, manual clients, any bundler

Bundle Size Impact

Client Type Without Config With Config Savings
Manual (no SDDM) ~2KB (runtime) ~2KB (runtime) 0KB (already optimal)
Generated (with SDDM) ~3.5KB (both paths) ~1.5KB (SDDM only) ~2KB saved

Implementation Checklist

  • [x] Create mapVariablesByTypeNames utility (✅ completed in this session)
  • [x] Test mapVariablesByTypeNames (✅ completed)
  • [ ] Create Upload-with-sddm.ts with SDDM implementation
  • [ ] Update Upload.ts with runtime implementation + __HAS_SDDM__ = false
  • [ ] Extract shared logic to helpers.ts
  • [ ] Add export condition to package.json
  • [ ] Export mapVariablesByTypeNames from utilities-for-generated
  • [ ] Add tests for both versions
  • [ ] Verify tree-shaking with bundle analysis
  • [ ] Document bundler configuration in docs

Related Work

Completed in this session:

  • ✅ Created src/docpar/core/sddm/mapVariables.ts (165 lines)
  • ✅ Implemented mapVariablesByTypeNames utility with visitor pattern
  • ✅ Added comprehensive tests (7 test cases, all passing)
  • ✅ Fixed Variables type to use unknown (src/lib/grafaid/request.ts:35-60)

Next steps:

  • Implement dual Upload extension files with export conditions

Alternative Considered: Generator Emits Inline Extension

We considered having the generator emit an optimized Upload extension inline in generated client code. Rejected because:

  • ❌ Doesn't help manual clients
  • ❌ More generator complexity
  • ❌ Harder to maintain (two different implementations)
  • ✅ Export conditions work for both generated AND manual clients

Priority: Medium
Estimated Effort: 1-2 days
Bundler Compatibility: Vite 2.0+, Webpack 5+, Rollup 2+, esbuild 0.14+, Parcel 2+

jasonkuhrt avatar Oct 29 '25 00:10 jasonkuhrt