Skip to content

Refactor ParsedMetadata as generic#324

Merged
daweifeng-replit merged 1 commit into
mainfrom
dawei/06-12-refactor_parsedmetadata_into_generic
Jun 16, 2025
Merged

Refactor ParsedMetadata as generic#324
daweifeng-replit merged 1 commit into
mainfrom
dawei/06-12-refactor_parsedmetadata_into_generic

Conversation

@daweifeng-replit

@daweifeng-replit daweifeng-replit commented Jun 13, 2025

Copy link
Copy Markdown
Contributor

Why

This PR makes the ParsedMetadata type a generic parameter rather than a global interface that requires declaration merging. This allows for having multiple ParsedMetadata for different projects in a monorepo

What changed

  • Changed ParsedMetadata from a global interface to a generic parameter in relevant types
  • Updated ProcedureHandlerContext to accept a generic ParsedMetadata parameter
  • Modified ServerHandshakeOptions to accept a generic ParsedMetadata parameter
  • Updated ServerTransport to accept generic parameters for metadata schema and parsed metadata
  • Update type parameters to procedure types (rpc, upload, subscription, stream)
  • Updated tests to use the new generic parameters

Example:

      const requestSchema = Type.Object({
        data: Type.String(),
      });

      interface ParsedMetadata {
        data: string;
        extra: number;
      }

      const clientTransport = getClientTransport(
        'client',
        createClientHandshakeOptions(requestSchema, () => ({ data: 'foobar' })),
      );
      const serverTransport = getServerTransport(
        'SERVER',
        createServerHandshakeOptions(requestSchema, (metadata) => {
          return {
            data: metadata.data,
            extra: 42,
          };
        }),
      );

      const services = {
        test: createServiceSchema<object, ParsedMetadata>().define({
          getData: Procedure.rpc({
            requestInit: Type.Object({}),
            responseData: Type.Object({
              data: Type.String(),
              extra: Type.Number(),
            }),
            handler: async ({ ctx }) => {
              return Ok({ ...ctx.metadata });
            },
          }),
        }),
      };
      const server = createServer(serverTransport, services);
      const client = createClient<typeof services>(
        clientTransport,
        serverTransport.clientId,
      );

Versioning

  • Breaking protocol change
  • Breaking ts/js API change

daweifeng-replit commented Jun 13, 2025

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@daweifeng-replit daweifeng-replit marked this pull request as ready for review June 13, 2025 00:37
@daweifeng-replit daweifeng-replit requested a review from a team as a code owner June 13, 2025 00:37
@daweifeng-replit daweifeng-replit requested review from Monkatraz, jackyzha0 and masad-frost and removed request for a team June 13, 2025 00:37
@daweifeng-replit daweifeng-replit changed the title Refactor ParsedMetadata into generic Refactor ParsedMetadata as generic Jun 13, 2025
Comment thread __tests__/cleanup.test.ts Outdated
const serverTransport = getServerTransport();
const handler =
vi.fn<(ctx: ProcedureHandlerContext<object, object>) => void>();
vi.fn<(ctx: ProcedureHandlerContext<object, object, unknown>) => void>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

weird that we do unknown in some places but object in others as the default value for this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's right. It should be object here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unknown where possible makes sense, object is basically any

Comment thread transport/server.ts

// invariant: must pass custom validation if defined
let parsedMetadata: ParsedMetadata = {};
let parsedMetadata: ParsedMetadata = {} as ParsedMetadata;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do we need an additional assertion here when we didnt before? same question for L330

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because ParsedMetadata extends object now. It extends Record<string, unknown> before

Comment thread transport/transport.test.ts
Comment on lines +115 to +118
// MetadataSchema and ParsedMetadata are not used in this test,
// so we can safely use any here
// eslint-disable-next-line @typescript-eslint/no-explicit-any
serverTransport: ServerTransport<Connection, any, any>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

still weird, feels like we should be using <Connection, TSchema, object> here which should work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, it doesn't work. The ServerTransport class has a method extendHandshake that takes ServerHandshakeOptions<MetadataSchema, ParsedMetadata>. These options include a validate function that expects the previous metadata to match the ParsedMetadata type. When we use any for ParsedMetadata, TypeScript allows any type to be passed. But when we try to use a specific type like { kept: string }, TypeScript enforces that the previous metadata must match that exact type 😢

const transports: Array<
WebSocketClientTransport | WebSocketServerTransport
// eslint-disable-next-line @typescript-eslint/no-explicit-any
WebSocketClientTransport | WebSocketServerTransport<any, any>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same comment as above

@jackyzha0

Copy link
Copy Markdown
Member

lgtm to unblock but i'd like to do a future pass at somepoint to see if we can figure out if we can thread less generics through everything :')

@daweifeng-replit daweifeng-replit force-pushed the dawei/06-12-refactor_parsedmetadata_into_generic branch from 6d9b233 to 1be6607 Compare June 13, 2025 18:56
@daweifeng-replit daweifeng-replit added enhancement New feature or request minor New features or functionality that are backwards compatible (e.g., 1.2.3 → 1.3.0) labels Jun 16, 2025

daweifeng-replit commented Jun 16, 2025

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jun 16, 6:51 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 16, 6:53 PM UTC: Graphite rebased this pull request as part of a merge.
  • Jun 16, 6:55 PM UTC: @daweifeng-replit merged this pull request with Graphite.

@daweifeng-replit daweifeng-replit changed the base branch from dawei/refactor-global-declare to graphite-base/324 June 16, 2025 18:51
@daweifeng-replit daweifeng-replit changed the base branch from graphite-base/324 to main June 16, 2025 18:52
@daweifeng-replit daweifeng-replit force-pushed the dawei/06-12-refactor_parsedmetadata_into_generic branch from 1be6607 to 301dfe1 Compare June 16, 2025 18:53
@daweifeng-replit daweifeng-replit merged commit 6202f72 into main Jun 16, 2025
8 checks passed
@daweifeng-replit daweifeng-replit deleted the dawei/06-12-refactor_parsedmetadata_into_generic branch June 16, 2025 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request minor New features or functionality that are backwards compatible (e.g., 1.2.3 → 1.3.0)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants