refactor(ts): moves type patching on message type level #212
Conversation
WalkthroughRefactors type patching from runtime application into compile-time wrapping of generated proto exports; removes runtime patch infrastructure (applyPatches, ServiceClientOptions, requiresTypePatching) and updates generators, SDK/client factories, transports, and tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
stalniy
force-pushed
the
refactor/type-patching
branch
from
bf5a45b to
40974ac
Compare
stalniy
force-pushed
the
refactor/type-patching
branch
from
40974ac to
03525bc
Compare
stalniy
force-pushed
the
refactor/type-patching
branch
from
03525bc to
e5d5284
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ts/script/fix-ts-proto-generated-types.ts`:
- Around line 72-76: The computed importPath uses relativePath(filePath, ...)
which uses the file path instead of its directory, producing off-by-one imports;
change the call in fix-ts-proto-generated-types.ts to compute the relative path
from the file's directory (e.g. use path.dirname(filePath) or an equivalent to
get the directory) when building importPath for
`${ROOT_DIR}/generated/protos/patches/${prefix}PatchMessage.ts`, and ensure any
required path module is imported; keep references to namespace, prefix,
importPath, relativePath, filePath, symbolName, imports and exports when making
the change.
In `@ts/test/functional/protoc-gen-customtype-patches.spec.ts`:
- Around line 15-19: The cleanup check in the afterEach uses access(outputDir,
fsConst.W_OK).catch(() => false) but access resolves to undefined on success
(falsy), so the condition never runs; update the afterEach to correctly detect
successful access by either awaiting access in a try/catch (call
rmdir(outputDir, { recursive: true }) in the try when access succeeds) or change
the promise handling to await access(...).then(() => true).catch(() => false)
before the if; reference the afterEach, access, outputDir, rmdir, and
fsConst.W_OK identifiers when making the change.
🧹 Nitpick comments (2)
ts/src/sdk/client/createServiceClient.spec.ts (1)
329-334: Remove stalerequiresTypePatchingfrom mock transport.The
requiresTypePatchingproperty does not exist in theTransportinterface (which only definesunaryandstreammethods). This line should be removed to keep the mock aligned with the actual type definition.♻️ Suggested fix
return { - requiresTypePatching: true, unary: notImplemented, stream: notImplemented, [responseType]: method, } as unknown as Omit<Transport, T> & Record<T, jest.MockedFunction<Transport[T]>>;ts/script/fix-ts-proto-generated-types.ts (1)
72-73: Hardcoded namespace mapping may need extension.The mapping
namespace === "akash" ? "node" : namespaceworks for current namespaces but requires manual updates for new namespaces that don't follow this convention. Consider extracting this to a configuration constant for easier maintenance.
stalniy
force-pushed
the
refactor/type-patching
branch
from
e5d5284 to
7ad294d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ts/script/protoc-gen-customtype-patches.ts`:
- Around line 129-135: The current generator builds patchesTypeFileName with
fileName.replace("CustomTypePatches","PatchMessage") and imports
`./${fileName}.ts`, which fails for custom output filenames and non-.ts
extensions; update the logic in the block that sets patchesTypeFileName,
patchTypeFile and the patchTypeFile.print calls (symbols: patchesTypeFileName,
fileName, patchTypeFile.print, and schema.options.importExtension) to 1)
validate fileName doesn't already collide with the intended PatchMessage name
and derive a safe PatchMessage basename using path.basename(fileName) and a
deterministic suffix (fail-fast if a collision would occur), 2) compute the
import path using the basename plus schema.options.importExtension (or default)
rather than hardcoding ".ts" and preserve relative directory semantics, and 3)
ensure the printed import uses the safe basename/importExtension so generated
imports are correct for custom filenames and extensions.
♻️ Duplicate comments (1)
ts/script/fix-ts-proto-generated-types.ts (1)
72-76: Bug:relativePathcomputed from file path instead of its directory.Line 74 uses
relativePath(filePath, ...)which computes the relative path from the source file itself rather than its containing directory. This produces incorrect import paths (off by one directory level).Additionally, for cross-platform compatibility, consider using
path.posix.relativeto ensure forward slashes in import paths on Windows.🐛 Proposed fix
-import { dirname, relative as relativePath, resolve as resolvePath } from "node:path"; +import { dirname, posix, resolve as resolvePath } from "node:path";And in
injectOwnHelpers(lines 54, 57) andapplyPatching(line 74):- const importPath = relativePath(filePath, `${ROOT_DIR}/generated/protos/patches/${prefix}PatchMessage.ts`); + const importPath = posix.relative(dirname(filePath), `${ROOT_DIR}/generated/protos/patches/${prefix}PatchMessage.ts`);Similarly update lines 54 and 57 to use
posix.relative(dirname(path), ...).
🧹 Nitpick comments (1)
ts/script/fix-ts-proto-generated-types.ts (1)
19-23: Consider adding error handling for missing patch files.If the glob matches no files or a patch file fails to import, this would silently result in an empty
typesToPatchset. Given the fail-fast preference for build scripts, consider adding validation that at least one patch file was processed, or logging when patches are loaded.💡 Optional enhancement
const typesToPatch = new Set<string>(); for await (const patchFile of fs.glob(`${ROOT_DIR}/generated/patches/*CustomTypePatches.ts`)) { const { patches } = await import(patchFile); Object.keys(patches).forEach((key) => typesToPatch.add(key)); } + +if (typesToPatch.size === 0) { + console.warn("Warning: No type patches found. Verify patch files exist."); +}Based on learnings, fail-fast behavior is preferred for build-time scripts.
| const patchesTypeFileName = fileName.replace("CustomTypePatches", "PatchMessage"); | ||
| const patchTypeFile = schema.generateFile(patchesTypeFileName); | ||
| patchTypeFile.print(`import { patches } from "./${fileName}";`); | ||
| patchTypeFile.print(`import type { MessageDesc } from "../../sdk/client/types.ts";`); | ||
| patchTypeFile.print(`export const patched = <T extends MessageDesc>(messageDesc: T): T => {`); | ||
| patchTypeFile.print(` const patchMessage = patches[messageDesc.$type as keyof typeof patches] as any;`); | ||
| patchTypeFile.print(` if (!patchMessage) return messageDesc;`); |
There was a problem hiding this comment.
Guard PatchMessage filename + honor importExtension/relative path.
Line 129 uses replace("CustomTypePatches", "PatchMessage"), which no-ops for custom output filenames (env var) and can collide with the patches file. Line 131 also imports ./${fileName} (including any subdirs) and hardcodes .ts, ignoring schema.options.importExtension. Both can break generated output when non-default filenames or extensions are used. Suggest validating the filename, deriving a safe PatchMessage filename, and importing by basename + importExtension.
🛠️ Suggested fix
- const patchesTypeFileName = fileName.replace("CustomTypePatches", "PatchMessage");
+ const patchesTypeFileName = fileName.replace(/CustomTypePatches\.ts$/, "PatchMessage.ts");
+ if (patchesTypeFileName === fileName) {
+ throw new Error(`Unexpected patches output filename: ${fileName}`);
+ }
const patchTypeFile = schema.generateFile(patchesTypeFileName);
- patchTypeFile.print(`import { patches } from "./${fileName}";`);
+ const patchesImport = basename(fileName).replace(/\.ts$/, "");
+ patchTypeFile.print(`import { patches } from "./${patchesImport}${importExtension}";`);Based on learnings, fail-fast behavior is preferred for build-time scripts.
🤖 Prompt for AI Agents
In `@ts/script/protoc-gen-customtype-patches.ts` around lines 129 - 135, The
current generator builds patchesTypeFileName with
fileName.replace("CustomTypePatches","PatchMessage") and imports
`./${fileName}.ts`, which fails for custom output filenames and non-.ts
extensions; update the logic in the block that sets patchesTypeFileName,
patchTypeFile and the patchTypeFile.print calls (symbols: patchesTypeFileName,
fileName, patchTypeFile.print, and schema.options.importExtension) to 1)
validate fileName doesn't already collide with the intended PatchMessage name
and derive a safe PatchMessage basename using path.basename(fileName) and a
deterministic suffix (fail-fast if a collision would occur), 2) compute the
import path using the basename plus schema.options.importExtension (or default)
rather than hardcoding ".ts" and preserve relative directory semantics, and 3)
ensure the printed import uses the safe basename/importExtension so generated
imports are correct for custom filenames and extensions.
📝 Description
moves type patching on message type level by post-processing ts-proto output. This makes the transport layer and the SDK unaware of patching issues. Depends on #211
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
I've updated relevant documentation📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]