-
Notifications
You must be signed in to change notification settings - Fork 5
fix: removeFMTableNames types #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@proofkit/fmdapi": patch | ||
| --- | ||
|
|
||
| Update removeFMTableNames function for proper type-safety |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,31 +1,29 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { S, L, U } from "ts-toolbelt"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type * as z3 from "zod/v3"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type * as z4 from "zod/v4/core"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type TransformedFields<T extends Record<string, any>> = U.Merge< | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [Field in keyof T]: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [Key in Field extends string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? L.Last<S.Split<Field, "::">> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : Field]: T[Field]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }[keyof T] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| >; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type StripFMTableName<K extends PropertyKey> = K extends `${string}::${infer R}` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? R | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : K; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function removeFMTableNames<T extends Record<string, any>>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type TransformedFields<T extends object> = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [K in keyof T as K extends string ? StripFMTableName<K> : K]: T[K]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function removeFMTableNames<T extends object>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj: T, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): TransformedFields<T> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newObj: any = {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newObj = {} as TransformedFields<T>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const key in obj) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (key.includes("::")) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const newKey = key.split("::")[1]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| newObj[newKey as keyof TransformedFields<T>] = obj[key]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| newObj[key] = obj[key]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const originalKey = key as keyof T; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const value = obj[originalKey]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const mappedKey = ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof key === "string" && key.includes("::") ? key.split("::")[1] : key | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) as keyof TransformedFields<T>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Use a temporary index signature cast to assign without any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (newObj as unknown as Record<PropertyKey, unknown>)[ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mappedKey as unknown as PropertyKey | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ] = value as unknown; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
16
to
27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Avoid copying inherited enumerable properties; restrict to own keys. for..in iterates inherited enumerable props. That can silently pull in prototype properties. Add a hasOwnProperty guard (minimal change), or iterate Object.keys/Reflect.ownKeys instead. Minimal, surgical fix: - for (const key in obj) {
+ for (const key in obj) {
+ if (!Object.prototype.hasOwnProperty.call(obj, key)) continue;
const originalKey = key as keyof T;
const value = obj[originalKey];If you want to also preserve symbol keys (currently dropped), consider Reflect.ownKeys: - for (const key in obj) {
- const originalKey = key as keyof T;
- const value = obj[originalKey];
+ for (const k of Reflect.ownKeys(obj)) {
+ const key = k as keyof T;
+ const value = (obj as Record<PropertyKey, unknown>)[k];…and keep the mappedKey logic the same (only transform when typeof k === "string"). 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return newObj; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { removeFMTableNames } from "../src"; | ||
|
|
||
| describe("removeFMTableNames", () => { | ||
| it("strips FM table prefixes from keys and preserves values", () => { | ||
| const input = { | ||
| "Customer::name": "Alice", | ||
| count: 2, | ||
| "X::flag": true, | ||
| } as const; | ||
|
|
||
| const out = removeFMTableNames(input); | ||
|
|
||
| expect(out).toEqual({ name: "Alice", count: 2, flag: true }); | ||
| expect("Customer::name" in (out as Record<string, unknown>)).toBe(false); | ||
| expect("name" in out).toBe(true); | ||
| expect(out.name).toBe("Alice"); | ||
| expect(out.count).toBe(2); | ||
| expect(out.flag).toBe(true); | ||
| }); | ||
|
|
||
| it("produces a type with stripped keys", () => { | ||
| type Input = { | ||
| "Customer::first_name": string; | ||
| last_name: string; | ||
| "Portal::recordId": number; | ||
| }; | ||
|
|
||
| const input: Input = { | ||
| "Customer::first_name": "Bob", | ||
| last_name: "Builder", | ||
| "Portal::recordId": 42, | ||
| }; | ||
| const out = removeFMTableNames(input); | ||
|
|
||
| // Type-level assertions via assignment | ||
| const first: string = out.first_name; | ||
| const last: string = out.last_name; | ||
| const recId: number = out.recordId; | ||
| expect(first).toBe("Bob"); | ||
| expect(last).toBe("Builder"); | ||
| expect(recId).toBe(42); | ||
|
|
||
| // Old keys should not exist on the resulting type | ||
| // @ts-expect-error - old FM-prefixed key should not exist on result type | ||
| out["Customer::first_name"]; | ||
| // @ts-expect-error - old FM-prefixed key should not exist on result type | ||
| out["Portal::recordId"]; | ||
| }); | ||
|
Comment on lines
+22
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chaints-expect-error in runtime tests isn’t enforced by Vitest; wire up the dedicated type-check instead. Vitest (via esbuild) doesn’t type-check by default, so the @ts-expect-error assertions here won’t fail builds. You already added a dedicated type-check config—great. Ensure CI runs it. Run locally/CI: Optionally, move the @ts-expect-error assertions to packages/fmdapi/tests/type/removeFMTableNames.typecheck.ts to keep runtime tests focused. 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Type-check only:
npx -y tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmitLength of output: 556 Type-check setup is broken – CI will fail until it’s fixed The dedicated type-check command (
You need to:
Once the above is addressed, ensure your CI invokes: npx tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmitso that any stray or missing 🤖 Prompt for AI Agents |
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "extends": "../tsconfig.json", | ||
| "compilerOptions": { | ||
| "noEmit": true, | ||
| "types": ["vitest", "node"], | ||
| "isolatedModules": true | ||
| }, | ||
| "include": ["./**/*.ts"], | ||
| "exclude": ["../dist", "../schema"] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| { | ||
| "compilerOptions": { | ||
| "strict": true, | ||
| "target": "ES2022", | ||
| "module": "ESNext", | ||
| "moduleResolution": "Bundler", | ||
| "jsx": "react-jsx", | ||
| "skipLibCheck": true, | ||
| "noEmit": true, | ||
| "isolatedModules": true, | ||
| "types": ["node"] | ||
| }, | ||
| "files": ["./type/removeFMTableNames.typecheck.ts"] | ||
|
Comment on lines
+2
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainEnsure your toolchain TS version supports moduleResolution: "Bundler". Bundler resolution requires a relatively recent TypeScript. Please confirm your CI/runner uses a compatible TS version; otherwise, this config will be ignored or fail. You can verify with: 🏁 Script executed: #!/bin/bash
# Expect TS >= 5.x supporting "Bundler" moduleResolution.
npx -y tsc -v
# Also prove the typecheck config compiles:
npx -y tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmitLength of output: 677 Fix TypeScript type‐check configuration errors Verification shows your CI is using TypeScript 5.9.2, so • TS2688: Cannot find type definition file for ‘node’. To resolve:
After making these fixes, re-run: npx tsc -v
npx tsc -p packages/fmdapi/tests/tsconfig.typecheck.json --noEmit🤖 Prompt for AI Agents |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Type/runtime mismatch: StripFMTableName uses “last ::” while runtime uses “first ::”.
Currently, the type-level mapper strips after the last '::' (due to
${string}::${infer R}greediness), while the runtime useskey.split("::")[1](after the first). For keys containing multiple '::', the returned object’s keys won’t match its static type.Pick ONE of the following to align behavior:
Option A — make runtime match the type (strip after the last '::'):
Option B — make the type match the runtime (strip after the first '::'):
If you have domain certainty that keys never contain more than one '::', add a test to lock that invariant; otherwise, I recommend Option A (safer in the presence of extra qualifiers).
📝 Committable suggestion