narrow AnyType to StringType in mapped types#2412
Conversation
| keyListType instanceof NumberType || | ||
| keyListType instanceof SymbolType | ||
| keyListType instanceof SymbolType || | ||
| keyListType instanceof AnyType |
There was a problem hiding this comment.
should we do the same for unknown? what are all valid ts.types to index an object?
There was a problem hiding this comment.
Ah good point. With this code:
type Foo = {
[key: boolean]: unknown;
};This is the error I get:
An index signature parameter type must be 'string', 'number', 'symbol', or a template literal type.
But afaik, everything gets coerced to a string anyways in TS. Eg:
$ node
Welcome to Node.js v20.18.0.
Type ".help" for more information.
> t = {};
{}
> t[1] = 'the number one';
'the number one'
> t[1]
'the number one'
> t['1']
'the number one'
> t
{ '1': 'the number one' }
> t['1'] = 'the string one'
'the string one'
> t
{ '1': 'the string one' }
> t[undefined] = 'this was undefined'
'this was undefined'
> t[undefined]
'this was undefined'
> t['undefined']
'this was undefined'
So I would think that this perhaps could just never throw, and instead always default to string? As-is, this it kind of seems like an additional, more restrictive typecheck.
There was a problem hiding this comment.
Can't keys be symbols (in JS and TS) or string literals (in TS) as well? So it's not always string, right?
There was a problem hiding this comment.
This is the full list according to the error message:
An index signature parameter type must be 'string', 'number', 'symbol', or a template literal type.
But I thought pretty much anything can be a key due to toString behavior. eg:
$ node
Welcome to Node.js v20.18.0.
Type ".help" for more information.
> t = {};
{}
> obj = { hello: 'there'};
{ hello: 'there' }
> t[obj] = 'object value';
'object value'
> t
{ '[object Object]': 'object value' }
> obj.toString()
'[object Object]'
It looks like Symbol does make it into the object, but it doesn't survive JSON.stringify(). Wow, TIL.
I'm not sure what makes the most sense here. I'm pretty new to working with the TS AST like this. Happy to try whatever you think makes sense.
There was a problem hiding this comment.
I think start by making more tests cases with more types (ideally in the same test example rather than creating a ton of examples).
There was a problem hiding this comment.
Symbol here:
> s = Symbol('symb');
Symbol(symb)
> s
Symbol(symb)
> t[s] = 'symbol value';
'symbol value'
> t
{ '[object Object]': 'object value', [Symbol(symb)]: 'symbol value' }
> JSON.stringify(t)
'{"[object Object]":"object value"}'
>
By "ideally in the same test example rather than creating a ton of examples" do you mean just in a single main.ts and schema.json file?
There was a problem hiding this comment.
I realized that you guys had already covered most of these, and in a pattern that I was ignoring.
number is handled here:
symbol is handled here:
I copied this for any and for a template literal (which I think are the last two that are uncovered, based on this:
An index signature parameter type must be 'string', 'number', 'symbol', or a template literal type.
I stuck both of these in their own files, to keep the pattern going. Lmk if you'd prefer them in a single file. I confirmed that Record<any, string> is failing without this change.
I feel like this shouldn't need to handle unknown, since that isn't valid TS I don't think.
|
LGTM |
|
🚀 PR was released in |
github-actions
Bot
added
released
3 participants
Overview
The prosemirror library has this bit, which uses
anyas an index type:This is causing the error shown below when processed by
ts-json-schema-generator.To try and fix this, I'm trying to narrow it to a string.
The error this test generates before the fix:
Version
Published prerelease version:
v2.5.0-next.14Changelog
🎉 This release contains work from new contributors! 🎉
Thanks for all your work!
❤️ Sam Sudar (@srsudar)
❤️ Orta Therox (@orta)
❤️ James Vaughan (@jamesbvaughan)
❤️ Alex (@alexchexes)
❤️ Cal (@CalLavicka)
❤️ Valentyne Stigloher (@pixunil)
🚀 Enhancement
NewExpressionparser #2346 (@jamesbvaughan)🐛 Bug Fix
--additional-propertiesoption #2305 (@alexchexes)--type "*"is used with multiple exports #2284 (@alexchexes @arthurfiorette)symbol#2282 (@alexchexes)🔩 Dependency Updates
Authors: 9