Skip to content
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

fix: Support template and number keys in schemas #3444

Closed
wants to merge 1 commit into from
Closed

fix: Support template and number keys in schemas #3444

wants to merge 1 commit into from

Conversation

GrantGryczan
Copy link

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

Comment on lines -521 to -524
: // for a recursive union type, the child will never extend the parent type.
// but the parent will still extend the child
Type extends Type[Key]
? [Key]
Copy link
Author

@GrantGryczan GrantGryczan Oct 11, 2022

Choose a reason for hiding this comment

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

This prevented template string keys from working because, for example, if Type is { a: Record<`example-${number}`, any> } and Key is 'a', then this condition succeeds, so it outputs ['a'] instead of ['a', `example-${number}`] like it's supposed to, despite there being no recursion there.

This fact is exclusive to records with number or template string keys because those allow for infinite possible key values, which apparently informs TypeScript that it should allow any keys on the object.

That's why I had to change some of the recursive typing support here. The new method I used is more robust anyway since it checks for truly identical expressions rather than merely parent extending child or child extending parent (neither of which necessarily imply recursion).

@baileympearson
Copy link
Contributor

baileympearson commented Oct 11, 2022

Hey @GrantGryczan, thanks for the contribution. At this time, we're not accepting changes to our types. After #3433 merges, we plan on leaving TS as-is in the 4.x versions of the driver, and are reconsidering the level of Typescript support we offer in the next major version. We'll keep this PR in mind for the future, in case we consider reintroducing support for dot notation in the future.

@GrantGryczan
Copy link
Author

GrantGryczan commented Oct 12, 2022

Thanks for letting me know @baileympearson. Though I'm very concerned because this bug is giving me countless type errors in my project, preventing my production build from succeeding and making this package entirely unusable for me, as I heavily depend on number and template string keys in my Record types (which I would have elaborated on before un-drafting this PR).

Will those type errors go away after the aforementioned merge? If not, I would be happy to try to fix this again after the merge, if you would allow it. I'm going to have to use my fork of this repo otherwise, which is very unpreferable since then I don't automatically get continual support via updates.

@GrantGryczan
Copy link
Author

GrantGryczan commented Oct 16, 2022

A couple days and still no response, so I'll ask again: what should I do in order for this package to be usable for me? Should I submit a new PR now that #3433 has merged, or keep using my unmaintained fork of this repo? This is a fix for a bug that blocks my production build, not just a "change to your types". It's obviously not an option for me to just ignore the countless type errors this causes. I need to do something about this in order to continue development.

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.

2 participants