Add RenameKeys type#1435
Conversation
|
I think this is close, but I would not merge it with collision support as-is. The collision behavior is unsafe. The tests currently say that renaming type Result = RenameKey<{a?: number; b: string}, 'a', 'b'>;This becomes const value: Result = {};That means renaming an optional key over a required key accidentally makes the required key optional. I think the simpler and better contract is to not support target-key collisions, except maybe renaming a key to itself. Supporting collisions correctly would need a lot more subtle modifier rules, and I don't think the use-case is worth it. There is also a small issue with type Result = RenameKeys<{a: number; b: string}, {a?: 'alpha'}>;This ends up as Minor: there is a new unused |
|
Thanks! That makes sense. I reworked it to drop collision support entirely and expanded tests with positive + negative coverage for a bunch of edge cases. The comments explain them. Also, a consequence of this is that since the check is I was also wondering about a Re the lint: the lone blocks are intentional as each case block-scopes a reused local |
som-sm
left a comment
There was a problem hiding this comment.
These test cases fail currently:
type T1 = RenameKeys<{a: 1; b: 2}, {a: 'x'} | {b: 'y'}>;
//=> {a: 1; b: 2}
type T2 = RenameKeys<{a: 1; b: 2}, {a: 'b' | 'c'}>; // Doesn't error, but should
//=> {b: 1 | 2; c: 1}
type T3 = RenameKeys<{[x: string]: number; a: 1}, {a: 'b'}>; // Errors, but shouldn't|
I'll work on all this feedback and set some stuff up in a TypeScript playground link so we can iterate more quickly before I push. Ended up testing things in vscode itself... |
type now checks in the body instead of the params so I can check for various conditions that have to be met before a rename is safe to do
|
I pushed a redesign that addresses everything in the thread. V imp. The constraint is now structural-only (
export type RenameKey<BaseType, FromKey extends KeysOfUnion<BaseType>, ToKey extends PropertyKey> = RenameKeys<
BaseType,
{ [Key in FromKey]: ToKey }
>;The pair-of-surfaces pattern already exists in type-fest: Two cases the constraint can't catch and the type doesn't reject cleanly so I short-circuit directly:
|
|
If I were to open a PR that sets up husky to run tests before push and lint-staged before commit, would that be merged? I feel that should definitely be there in a library like this. |
No, I don’t think we want to add that. These tools add unnecessary friction, and blocking commits tends to make contributing more annoying than helpful. Contributors should be able to run checks locally when it makes sense, with CI checks acting as the final gate. |
|
Yeah, I don't want that. I had those in the past in other projects and caused more problems than they saved. |
|
Few thoughts:
IMO, returning |
|
Hmmmmm. I do agree that For missing keys ( For collisions tho ( Maybe we go lenient on missing keys like Omit, and keep the never, so strict, on collisions, duplicate targets, basically anything that is "no this should not be allowed" vs "there isn't really a problem with not doing anything about this" |
|
@materwelonDhruv How hard do you think it would be to handle collisions? I believe these are the only cases we need to consider when multiple properties
|
|
@som-sm just to make sure I'm reading this right. You want these to be valid and produce a merged result, not return Do you think that has valid use cases? This is basically imitating a merge, right? Also, are you sure this being allowed is a good idea? We should guard against this kind of rename right? Why would someone want to do that? Why should someone want to rename keys this way? |
Yup, correct!
Yeah kinda, just that merge is limited to only two items, this can happen with any no. of items. There could be several valid use cases for merging. Here are some AI-suggested ones that look pretty reasonable: type SearchInput = {
textQuery: string;
voiceQuery: Blob;
imageQuery: File;
};
type Normalized = RenameKeys<SearchInput, {textQuery: 'query'; voiceQuery: 'query'; imageQuery: 'query'}>;
//=> {query: string | Blob | File}
type LegacyUser = {
oldId?: string;
numericId: number;
name: string;
};
type Migrated = RenameKeys<LegacyUser, {oldId: 'id'; numericId: 'id'}>;
//=> {id: string | number; name: string} |
|
oki imma try. |
|
We just came full circle lmao. Because I think @sindresorhus explicitly asked to reject such collisions where we end up in a union 😅 Some tests that currently reject this behavior will change now. |
This was an issue because the merged result was incorrect; it would have been fine if the result were And also, there aren't many cases here, so I guess it's worth solving it!
That's fine! |
|
okay that makes sense. I'm nearly done with it. testing with the cases you gave and some others of my own. and also simplifying the implementation a bit. |
som-sm
left a comment
There was a problem hiding this comment.
This currently fails:
type T = RenameKeys<{a: string}, {a: 'b' | 'c'}>;
//=> Current: never
//=> Expected: {b: string; c: string}
som-sm
left a comment
There was a problem hiding this comment.
Doesn't seem to add | undefined when exactOptionalPropertyTypes is disabled, refer:
type T1 = RenameKeys<{a?: 1; b: 2}, {b: 'x'; a: 'x'}>;
// Current: {x: 1 | 2}
// Expected: {x: 1 | 2 | undefined}
type T2 = RenameKeys<{a?: 1; b: 2}, {b: 'a'}>;
// Current: {a: 1 | 2}
// Expected: {a: 1 | 2 | undefined}
type T3 = RenameKeys<{a?: 1; b: 2}, {a: 'b'}>;
// Current: {b: 1 | 2}
// Expected: {b: 1 | 2 | undefined}|
Fixed. The All three of your cases should now return the correct I'm gonna check if some of my new impl can be replaced by any other type-fest types first before I commit. And one other thing. Optional jeys show an explicit |
|
I can add a folder outside test-d/ for EOPT off tests. But that's outside the files this PR is currently about. So better to ask first |
also refactored to reuse some existing types
RenameKey and RenameKeys typesRenameKeys type
It's fine, you don't need to add tests for
That's fine too, with |
|
I already verified it manually 👍🏻 so it should be good on that front. |
and also add additional test cases
|
bump @som-sm :) |
|
I'm testing that simplified version right now btw. Will reply to your review comments in a bit. |
|
Posted a comment above @som-sm |
Yeah, sure! |
|
OKAY. I think I'm done. Gonna push now. Also added like 3 more tests. |
|
I'm trying one other thing. If it's straightforward to get autocomplete on the second arg's map for the target keys. If it's too complex, it won't be worth it. Edit: Not worth it. |
|
I like how this started with this 😭: export type RenameKey<
BaseType,
FromKey extends KeysOfUnion<BaseType>,
ToKey extends PropertyKey,
> = {
[Key in keyof BaseType as Key extends FromKey ? ToKey : Key]: BaseType[Key];
};Nice PR review. V good learning experience. Thanks som. |
|
Maybe worth it to add CodeRabbit and Greptile code reviews to the repo? Both are free for OSS. |

Renames one or more keys in an object type while preserving each value type and the
?/readonlymodifiers. Distributes over union object types.RenameKeyis the single-key form;RenameKeystakes a map of old-to-new names for batch renames. Useful for deriving a JSON-shape from an internal type (camelCase to snake_case), relabeling a discriminator across a tagged union, or any spot whereMerge/OverridePropertieswould let you change a value type but not the key itself, as examples.I use this in my framework, seedcord's, docs site, where I have a
WithDeprecationStatusmixin that attaches adeprecationStatusfield to entity types (classes, methods, properties). When rendering a child member of a deprecated parent, the section component needs to know the parent's status without claiming the child itself is deprecated, so I deriveWithParentDeprecationStatus = RenameKey<WithDeprecationStatus, 'deprecationStatus', 'parentDeprecationStatus'>. Because of this, it's just one line and has no risk of the two types drifting. And renaming the source key automatically flags every downstream rename if I ever touch the mixin.Some other examples below: