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

resolving readJson to something other than unknown #6438

Closed
mfulton26 opened this issue Jun 23, 2020 · 3 comments
Closed

resolving readJson to something other than unknown #6438

mfulton26 opened this issue Jun 23, 2020 · 3 comments

Comments

@mfulton26
Copy link

mfulton26 commented Jun 23, 2020

current types for JSON.parse return any so when parsing JSON it is easy to tell TS what type the parsed JSON is:

const data: Data = JSON.parse(text);

readJson, however, resolves to unknown so things are a bit more complicated:

const data: Data = await readJson(filePath);
//    ^^^^ Type 'unknown' is not assignable to type 'Data'.

this can be worked around but I think it makes the code clunky:

const json = await readJson(filePath);
const data: Data = json as Data;

I propose one of the following:

  1. change readJson to resolve to any (to be consistent with JSON.parse, some could argue that JSON.parse shouldn't return any though)

  2. change readJson to resolve to T where T = unknown and can be specified or inferred:

    const data =  readJson<Data>(filePath);

    or

    const data: Data = readJson(filePath);
  3. same as 2. except T defaults to any (for consistency with JSON.parse)

personally I like 2. but unknown is inconsistent with current type definitions of JSON.parse and I typically favor consistency so I suggest 1. or 3.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Jun 23, 2020

I think unknown or Object is more correct here. We try to match built-ins usually, but TS's types for built-ins are a different story IMO. They return any in JSON.parse() for ease of use so you don't have to keep casting (microsoft/TypeScript#15225 (comment)). We should allow ourselves to dis-align with that.

This is not a place to use generics, see #5738 (comment).

@mfulton26
Copy link
Author

I think unknown is better than Object only because JSON.parse can return an array, a number, etc. (i.e. things that aren't objects)

I just realized that the type assertion/cast can be done inline so maybe this isn't as much of an issue as I originally thought

const data = await readJson(filePath) as Data;

closing but feel free to reopen if there's more to do here

thank you

@kitsonk
Copy link
Contributor

kitsonk commented Jun 23, 2020

Object is almost never correct... object is almost always more correct, but in the case of parsed JSON, it is really imperfect. Don't forget "foo", 123, true are valid JSON which pass JSON.parse, so if you had:

123

In a file, it would load and parse and return as a number.

Also, the type for any is likely a bit outdated in the TypeScript library, because it was done before unknown existed. If they had to do it all over again, it might end up being unknown. It is the most appropriate type for it at the moment in my opinion.

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

No branches or pull requests

3 participants