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

Make JsonRecord type safe #1693

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thewilkybarkid
Copy link
Contributor

@thewilkybarkid thewilkybarkid commented Apr 6, 2022

The JsonRecord type currently allows you to access any string key, but this isn't type safe. Looking at the commit history the type came from microsoft/TypeScript#1897 (comment), and does seem to have been copied over to many other libraries.

This PR changes the values of a JsonRecord to also possibly be undefined, covering the fact that the key might not be set.

This does mean that it's possible to set keys as undefined, even though this type doesn't exist in JSON (stringify will throw these keys away). This actually fixes my problem in gcanti/io-ts#628 (comment) (which was complicated to try and get round in gcanti/io-ts#639).

@thewilkybarkid
Copy link
Contributor Author

@thewilkybarkid
Copy link
Contributor Author

thewilkybarkid commented Apr 6, 2022

dtslint failed in CI; locally it was only running up to TS 4.1. Looks like the library isn't pinned so I had an outdated version.

@gcanti
Copy link
Owner

gcanti commented Apr 12, 2022

The JsonRecord type currently allows you to access any string key, but this isn't type safe

IIRC there's a tsconfig option for that (noUncheckedIndexedAccess)

@thewilkybarkid
Copy link
Contributor Author

thewilkybarkid commented Apr 12, 2022

@gcanti Ah, that's definitely something to enable generally (and does fix the accessing problem). Thanks.

It doesn't, however, get around gcanti/io-ts#628 (comment). Should I close this and reopen gcanti/io-ts#639?

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