Skip to content

Speed up codebase type-checking by collapsing dispatcher path overloads#800

Merged
dahlia merged 5 commits into
fedify-dev:mainfrom
2chanhaeng:dispatcher-path-types
Jun 10, 2026
Merged

Speed up codebase type-checking by collapsing dispatcher path overloads#800
dahlia merged 5 commits into
fedify-dev:mainfrom
2chanhaeng:dispatcher-path-types

Conversation

@2chanhaeng

@2chanhaeng 2chanhaeng commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Fixes #613.

Background

The setObjectDispatcher(), setCollectionDispatcher(), and setOrderedCollectionDispatcher() methods on FederationBuilderImpl typed their path parameter as RFC 6570 template-literal unions. Rfc6570Expression<TParam> is an 8-member union, and these overloads interpolated it up to four times, so each path type expanded to a Cartesian product of up to $8^4 = 4096$ template-literal variants.

TypeScript spent almost all of deno check's time structurally comparing these enormous unions. The cost was not confined to a single file: the context boundary checks (ContextImpl/FederationImpl vs Context/Federation) dragged these path types in throughout the type graph, so the combinatorial blowup propagated across the whole project.

Change

Collapse the affected dispatcher overloads on FederationBuilderImpl to a single path: string signature:

  • setObjectDispatcher() — removed 6 template-literal overloads (interpolated Rfc6570Expression up to three times, 8^3 = 512 variants, plus several simple multi-variable forms).
  • setCollectionDispatcher() and setOrderedCollectionDispatcher() — removed the template-literal overloads (up to 8^4 = 4096 variants each).

The net diff is 183 deletions in a single file (packages/fedify/src/federation/builder.ts).

Public API is unchanged

The template-literal overloads are kept on the public Federatable interface (and thus on Federation and FederationBuilder, which extend it). Users still get the same compile-time path validation and autocompletion. Only the internal implementation class signature was collapsed; because path: string is a wider, compatible implementation signature, it still satisfies the interface's narrower overloads.

Nothing is lost at runtime

Path validity—including the "one or more variables" requirement—is still enforced at runtime via assertPath() and Router.variables() (backed by @fedify/uri-template), exactly as before. The only thing removed is the costly compile-time RFC 6570 shape checking.

Impact

Full-codebase type-checking (mise run check:types) drops from ~99s to ~13s.

The setCollectionDispatcher() and setOrderedCollectionDispatcher()
overloads typed `path` as RFC 6570 template-literal unions:
`Rfc6570Expression` is an 8-member union, and the overloads interpolated
it up to four times, so each `path` type expanded to a Cartesian product
of up to 8^4 = 4096 template-literal variants.  TypeScript spent almost
all of `deno check`'s time structurally comparing these types, making a
cold check of middleware.ts take on the order of ~30s.

Collapse those overloads to a single `path: string` signature in both
the Federatable interface and FederationBuilderImpl.  Path validity is
still enforced at runtime via @fedify/uri-template (isPath() and
Router.variables(), already used in builder.ts), so nothing is lost
except the costly compile-time RFC 6570 shape checking.

fedify-dev#613

Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Codex:gpt-5.5
Same root cause as the previous commit: setObjectDispatcher() still
typed `path` as RFC 6570 template-literal unions.  Its widest overloads
interpolated `Rfc6570Expression` (an 8-member union) up to three times,
expanding `path` to 8^3 = 512 template-literal variants, plus several
simple multi-variable forms.  These were the last remaining
combinatorial dispatcher path types, and the context boundary checks
(ContextImpl/FederationImpl vs Context/Federation) dragged them in
throughout the type graph.

Collapse the setObjectDispatcher() overloads to a single `path: string`
signature in both the Federatable interface and FederationBuilderImpl.
Path validity (including the "one or more variables" requirement) is
still enforced at runtime via assertPath() and Router.variables() in the
existing implementation.

This roughly halves the remaining cold project type-check: locally
`deno task check:types` drops from ~24s to ~12s, on top of the ~99s ->
~24s from the previous commit.

fedify-dev#613

Assisted-by: Claude Code:claude-opus-4-8
Assisted-by: Codex:gpt-5.5
@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: dc4e3f3a-d67f-467b-bdf7-4ea9501c3541

📥 Commits

Reviewing files that changed from the base of the PR and between ee801b8 and bb2ef0a.

📒 Files selected for processing (2)
  • CHANGES.md
  • packages/fedify/src/federation/builder.ts

📝 Walkthrough

Walkthrough

The PR consolidates three public dispatcher registration method signatures in FederationBuilder by replacing template-literal path overloads with a single path: string parameter; it also updates CHANGES.md with a Version 2.3.0 entry and related reference fixes.

Changes

Dispatcher method signature consolidation

Layer / File(s) Summary
Consolidate dispatcher method signatures
packages/fedify/src/federation/builder.ts
setObjectDispatcher, setCollectionDispatcher, and setOrderedCollectionDispatcher accept path: string instead of template-literal overload variants. Methods retain runtime assertPath validation and derive/forward route variables via Router.variables() or internal casts.
Changelog entry and references
CHANGES.md
Adds Version 2.3.0 bullet describing the TypeScript type-checking improvement and adds/moves issue reference definitions including [#800].

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing RFC 6570 template-literal overloads from dispatcher methods to improve type-checking performance.
Description check ✅ Passed The description is detailed and directly related to the changeset, explaining the performance problem, solution, public API preservation, and measured impact.
Linked Issues check ✅ Passed The PR meaningfully addresses the primary coding objective of #613 by removing costly RFC 6570 template-literal overloads and demonstrating substantial type-check time reduction (99s→13s), though it is a partial fix for the broader performance investigation goals.
Out of Scope Changes check ✅ Passed All changes are in-scope: builder.ts removes the specified overloads, and CHANGES.md documents the optimization—no unrelated modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@2chanhaeng 2chanhaeng requested review from dahlia and sij411 and removed request for dahlia June 9, 2026 09:32

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request simplifies the method signatures of setObjectDispatcher, setCollectionDispatcher, and setOrderedCollectionDispatcher in FederationBuilderImpl by removing several complex template literal type overloads for the path parameter. I have no feedback to provide as there are no review comments.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@dahlia dahlia left a comment

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.

Although it's not a hard change, but since speed-ups are good news, we should add an entry about this speed-up to CHANGES.md.

Record in CHANGES.md that TypeScript type-checking was sped up by
simplifying the internal `path` parameter types of the object,
collection, and ordered collection dispatcher methods, dropping a
full codebase type check from ~99s to ~13s.

fedify-dev#613
fedify-dev#800

Assisted-by: Claude Code:claude-opus-4-8
@2chanhaeng

Copy link
Copy Markdown
Contributor Author

Although it's not a hard change, but since speed-ups are good news, we should add an entry about this speed-up to CHANGES.md.

Addressed in 878ab20.

dahlia
dahlia previously approved these changes Jun 9, 2026
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines Coverage Δ
packages/fedify/src/federation/builder.ts 60.68% <ø> (ø)

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sij411 sij411 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can reproduce that this meaningfully improves the expensive type-check phase, so I agree this is relevant to #613.

In my local A/B check against the PR base, the workspace deno check phase got much faster after this change. So this does address one real source of the slowdown.

That said, I would be careful about saying this fully fixes #613. The issue asks for the main slowdown sources to be understood and documented, and for check-all/pre-commit validation to become meaningfully faster without reducing coverage. This PR identifies and improves one major source, but it does not by itself show that the remaining check-all cost is understood, nor does it include a reusable benchmark/diagnostic artifact that future changes can compare against.

There is also a subtle API-boundary point worth documenting. Calls through the supported factory path still preserve the public overloads:

createFederationBuilder<void>().setObjectDispatcher(
  Note,
  "/notes/{id}",
  (_ctx, values) => {
    values.id satisfies string;
    // @ts-expect-error still rejected
    values.missing;
    return null;
  },
);

But direct use of the implementation class no longer has that compile-time protection:

  new FederationBuilderImpl<void>().setObjectDispatcher(
    Note,
    "/notes/{id}",
    (_ctx, values) => {
      // @ts-expect-error now unused
      values.missing;
      return null;
    },
  );

This seems acceptable if FederationBuilderImpl is intentionally internal; it is not re-exported from @fedify/fedify or @fedify/fedify/federation. But I think the PR description should say that explicitly: the public FederationBuilder/Federatable surface is unchanged, while direct source-level use of FederationBuilderImpl moves those checks from compile time to runtime.

So I would suggest either:

  • clarify that this is a partial fix for #613 focused on the dispatcher overload hot path; and
  • add a small type-level regression test for the supported factory/public path, or document FederationBuilderImpl as internal.

2chanhaeng added a commit to 2chanhaeng/fedify that referenced this pull request Jun 10, 2026
Document on setObjectDispatcher(), setCollectionDispatcher(), and
setOrderedCollectionDispatcher() that the RFC 6570 template-literal
`path` overloads were removed for type-checking efficiency, so URI
variable types can no longer be inferred from `path` and the variable
name should be supplied through the `TParam` generic argument instead.

Also clarify in CHANGES.md that this is a partial fix for the issue
below, targeting the dispatcher overload hot path; other contributors
to check-all cost may remain.

fedify-dev#613
fedify-dev#800

Assisted-by: Claude Code:claude-opus-4-8
Document on setObjectDispatcher(), setCollectionDispatcher(), and
setOrderedCollectionDispatcher() that the RFC 6570 template-literal
`path` overloads were removed for type-checking efficiency, so URI
variable types can no longer be inferred from `path` and the variable
name should be supplied through the `TParam` generic argument instead.

Also clarify in CHANGES.md that this is a partial fix for the issue
below, targeting the dispatcher overload hot path; other contributors
to check-all cost may remain.

Requested by @sij411 in fedify-dev#800 (review)

fedify-dev#613
fedify-dev#800

Assisted-by: Claude Code:claude-opus-4-8
@2chanhaeng 2chanhaeng force-pushed the dispatcher-path-types branch from ee801b8 to bb2ef0a Compare June 10, 2026 03:40
@2chanhaeng

Copy link
Copy Markdown
Contributor Author

I can reproduce that this meaningfully improves the expensive type-check phase, so I agree this is relevant to #613.

In my local A/B check against the PR base, the workspace deno check phase got much faster after this change. So this does address one real source of the slowdown.

That said, I would be careful about saying this fully fixes #613. The issue asks for the main slowdown sources to be understood and documented, and for check-all/pre-commit validation to become meaningfully faster without reducing coverage. This PR identifies and improves one major source, but it does not by itself show that the remaining check-all cost is understood, nor does it include a reusable benchmark/diagnostic artifact that future changes can compare against.

There is also a subtle API-boundary point worth documenting. Calls through the supported factory path still preserve the public overloads:

createFederationBuilder<void>().setObjectDispatcher(
  Note,
  "/notes/{id}",
  (_ctx, values) => {
    values.id satisfies string;
    // @ts-expect-error still rejected
    values.missing;
    return null;
  },
);

But direct use of the implementation class no longer has that compile-time protection:

  new FederationBuilderImpl<void>().setObjectDispatcher(
    Note,
    "/notes/{id}",
    (_ctx, values) => {
      // @ts-expect-error now unused
      values.missing;
      return null;
    },
  );

This seems acceptable if FederationBuilderImpl is intentionally internal; it is not re-exported from @fedify/fedify or @fedify/fedify/federation. But I think the PR description should say that explicitly: the public FederationBuilder/Federatable surface is unchanged, while direct source-level use of FederationBuilderImpl moves those checks from compile time to runtime.

So I would suggest either:

* clarify that this is a partial fix for #613 focused on the dispatcher overload hot path; and

* add a small type-level regression test for the supported factory/public path, or document `FederationBuilderImpl` as internal.

Addressed in bb2ef0a. Thanks!

@dahlia dahlia merged commit 74c8204 into fedify-dev:main Jun 10, 2026
17 checks passed
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.

Optimize deno task check-all performance without reducing pre-commit coverage

3 participants