Skip to content

Make KeyPaths not require @ts-ignore - #1842

Merged
jclem merged 6 commits into
mainfrom
fix-keypaths-type
Feb 7, 2022
Merged

Make KeyPaths not require @ts-ignore#1842
jclem merged 6 commits into
mainfrom
fix-keypaths-type

Conversation

@jclem

@jclemjclem commented Feb 4, 2022

Copy link
Copy Markdown
Contributor

Currently, KeyPaths requires a @ts-ignore line, because TypeScript can't verify that O[K] is a Record<string, unknown>.

In order to fix this, we can change KeyPaths<O extends Record<string, unknown>> to just KeyPaths<O>, but internally still verify that when recursing through O, we only recurse when O[K] extends Record<string, unknown>.

This makes it so that KeyPaths can be called on any value and I'm not sure if the repercussions of that are relevant. However, we should avoid @ts-ignore in types that are potentially consumed by projects using @primer/react, otherwise we essentially require that they have skipLibCheck: true in their TS config files.

There may be some better way of solving this, but my TypeScript knowledge isn't strong enough to figure it out 😄

@jclem
jclem requested review from a team and rezrahFebruary 4, 2022 20:52
@changeset-bot

changeset-botBot commented Feb 4, 2022

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 8701588

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actionsBot commented Feb 4, 2022

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js61.64 KB (0%)
dist/browser.umd.js62 KB (0%)

@colebemiscolebemis 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.

LGTM ✨

@rezrahrezrah 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.

Looks great, thanks @jclem!

@jclem
jclem enabled auto-merge (squash) February 7, 2022 17:36
@jclem
jclem merged commit 11011f5 into mainFeb 7, 2022
@jclem
jclem deleted the fix-keypaths-type branch February 7, 2022 17:38
@primer-cssprimer-css mentioned this pull request Feb 7, 2022
@jclem
jclem restored the fix-keypaths-type branch February 8, 2022 23:32
@joshblack
joshblack deleted the fix-keypaths-type branch January 19, 2023 16:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@jclem@colebemis@rezrah@siddharthkp