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

Stricter types for JSON #51003

Closed
brunocrosier opened this issue Sep 29, 2022 · 25 comments
Closed

Stricter types for JSON #51003

brunocrosier opened this issue Sep 29, 2022 · 25 comments

Comments

@brunocrosier
Copy link

lib Update Request

Configuration Check

My compilation target is ES2015 and my lib is the default.

Missing / Incorrect Definition

JSON.parse() and JSON.stringify()

The definitions for both of these functions currently accept a string, but this is not strict enough. Not all strings can be parsed or stringified. Many of these can be caught at a type level, meaning they could and should be added to the type definitions.

Sample Code

// @ts-expect-error
JSON.parse('')

// @ts-expect-error
JSON.parse("'foo'");

// @ts-expect-error
JSON.parse("[1, 2, 3, 4, ]");

// @ts-expect-error
JSON.parse('?')

// @ts-expect-error
JSON.parse("bar")

// @ts-expect-error
JSON.stringify({
    key: 2n
})

const circularReference: { myself?: any } = {};
circularReference.myself = circularReference;

// @ts-expect-error
JSON.stringify(circularReference)

TS Playground: https://www.typescriptlang.org/play?target=99#code/PTAEAEBcGcFoFMAeAHeBjSCBOWD2WAoAKQGUB5AOQDpkBDLaeACgHIWBKAgkCGBFdJng58xctToNmAIhYAzXLhbT2Abi48ocJKgzY8hUpRr1GTaQG0AjABpQAJjsBmOwBY7AXRXruYLf10hEUNxEylWAH4ODT8+HUF9USMJUxkAI3oVGN5tAT1hAzFjaEgsAEsAOwBzMrkATyYAbwJQVtAAa3g6gC4HCoIAX04CNFwKktA0Mqw0AFcAG3oAJXg5YXgKtHhextAAWzrGebkI3toKutAB0ABeUEaB9SmZheXV9c34KgOjuVvJ6ZzRZYFZrLAbLY+TRxPJBQrJKglcrVWoNZ5At5giHwThAA

Documentation Link

JSON
JSON.parse
JSON.stringify

@fatcerberus
Copy link

fatcerberus commented Sep 30, 2022

This doesn’t seem very useful; it only works if you’re passing string literals to JSON.parse()—it’s not possible to make a “JSON-compatible string” type without relying on generic inference—so the improved type checking is purely academic. Also, how do you propose to detect circular references at type level without excluding valid recursive structures?

@brunocrosier
Copy link
Author

brunocrosier commented Sep 30, 2022

This doesn’t seem very useful; it only works if you’re passing string literals to JSON.parse()—it’s not possible to make a “JSON-compatible string” type without relying on generic inference—so the improved type checking is purely academic. Also, how do you propose to detect circular references at type level without excluding valid recursive structures?

You are mistaken.

TS Playground

@fatcerberus
Copy link

fatcerberus commented Sep 30, 2022

That would exclude a ton of perfectly JSON-able objects. For example, tree structures.

@brunocrosier
Copy link
Author

That would exclude a ton of perfectly JSON-able objects.

For example?

@brunocrosier
Copy link
Author

brunocrosier commented Sep 30, 2022

@fatcerberus can you provide an example of something which this PR does not correctly parse or stringify?

Here you will find examples of a tree structure being parsed and stringified correctly:

@fatcerberus
Copy link

My point was, your types are excluding .myself on the basis that it could be a self-reference. That’s way too strict. As for JSON.parse, those stop being useful the minute you start passing string variables to it instead of hard-coded strings.

@brunocrosier
Copy link
Author

My point was, your types are excluding .myself on the basis that it could be a self-reference. That’s way too strict. As for JSON.parse, those stop being useful the minute you start passing string variables to it instead of hard-coded strings.

const emptyString = ''
JSON.parse(emptyString) // no error

image

@Josh-Cena
Copy link
Contributor

If you know your string statically at compile time, why do you need to parse it anyway?

@fatcerberus
Copy link

Cases in point:

const circularReference: { myself?: any } = {};
circularReference.myself = { foo: 812 };
JSON_Override.stringify(circularReference);  // error?!
const str = '{ "foo": "validJSON" }';  // pretend this was read from a file...
JSON_Override.parse(str);  // error. this is not useful.

The types only exist at compile time. It seems like you might not fully understand how the type system works?

@brunocrosier
Copy link
Author

brunocrosier commented Sep 30, 2022

Cases in point:

const circularReference: { myself?: any } = {};
circularReference.myself = { foo: 812 };
JSON_Override.stringify(circularReference);  // error?!
const str = '{ "foo": "validJSON" }';  // pretend this was read from a file...
JSON_Override.parse(str);  // error. this is not useful.

The types only exist at compile time. It seems like you might not fully understand how the type system works?

Why are you saying that JSON_Override.parse(str); is an error? You can see below, that is not the case:

image

If you have an example of where these proposed stricter types are actually incorrect, feel free to post a link to a reproducible TS Playground.

Also, I need to point out that saying things like "This doesn’t seem very useful" and "It seems like you might not fully understand how the type system works" is against this repository's Code of Conduct. Frankly, it's embarrassing. I urge you to rethink the way you interact with other human beings.

You are correct about the circular dependency point though, so I will amend the PR accordingly.

@fatcerberus
Copy link

fatcerberus commented Sep 30, 2022

let str = '{ "foo": "validJSON" }';
JSON_Override.parse(str);  // error

Like I said, these extra checks only work if you're passing literal-typed strings to parse. Excluding string outright is too strict, since there's no way to write a type for "JSON-compatible string".

@brunocrosier
Copy link
Author

let str = '{ "foo": "validJSON" }';
JSON_Override.parse(str);  // error

Like I said, these extra checks only work if you're passing literal-typed strings to parse.

you're just pasting code and adding // error

here is a TS Playground which shows that JSON_Override.parse(str); is not an error

If you can reproduce erroneous behaviour in TS Playground, feel free to send it.

@fatcerberus
Copy link

It absolutely is an error. You changed the let to const.

@brunocrosier
Copy link
Author

I used the const from your original comment

image

I can see that there is an error with let

I will investigate that

@nopeless
Copy link

If you know your string statically at compile time, why do you need to parse it anyway?

@Josh-Cena A case might be using JSON.parse to load large objects. Might be practical in browsers with low performance computers.

@nopeless
Copy link

@brunocrosier
https://262.ecma-international.org/6.0/#sec-json.parse
TL;DR: any JavaScript-like expression is a valid JSON

According to this, all of the values you provided should be parsable, including the empty string

@fatcerberus
Copy link

fatcerberus commented Sep 30, 2022

For the record, “this isn’t a useful change” and “this seems like a misunderstanding of how TypeScript works” are mere observations and don’t constitute CoC violations. I think it’s fair to criticize a feature request on those grounds, no?

Anyway: types are not values, and they exist only at compile time. The proposed checks for JSON.parse only work when the compiler has a way to know, at compile-time, exactly what is being passed to that function—because your proposed types, besides rejecting ill-formed JSON string literals, also disallow passing any variable typed as string. That’s not a useful behavior and would make the function very painful to use for almost all real-world use cases.

@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 30, 2022

Your implementation of "JSON_Override" wrongly prevents leading or tailing whitespace characters.

In general, as mentioned, this only works for string literals. A very narrow use case, but a whole buttload of added complexity, resulting in compilation slowdown and breaking changes (added global types). Especially such a generic name like CircularReference is a really bad idea.

@nopeless
Copy link

Your implementation of "JSON_Override" wrongly prevents leading or tailing whitespace characters.

In general, as mentioned, this only works for string literals. A very narrow use case, but a whole buttload of added complexity, resulting in compilation slowdown and breaking changes (added global types). Especially such a generic name like CircularReference is a really bad idea.

I'm not sure why CircularReference is a bad idea. As long as the context is clear in the type file, it shouldn't matter at all

@MartinJohns
Copy link
Contributor

It has such a generic name, odds are such a name is already in use, resulting in a breaking change. The introduction of such a utility type also means the team can't update it in the future (if a new feature would be better suited for such a type), without causing yet another breaking change (see Omit<>). And the team has the position to not add new utility types, unless they're required for declaration emit purposes. See #39522 (comment):

We've opted to not include utility type aliases in the lib unless they're required for declaration emit purposes.

@brunocrosier
Copy link
Author

brunocrosier commented Sep 30, 2022

You are right. At the moment, the types in this PR at the moment are too strict - particularly because of AllowedJSONParseText.

I still believe there is a lot of benefit in disallowing the parsing of an empty string, or the stringification of a bigint

What about something like this? It disallows values we know cannot be parsed / stringified (as opposed to the previous solution which would try to codify the allowed ones as a type and resolve to never otherwise)

TS Playground

@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 30, 2022

It doesn't address the problems with the utility types I mentioned.

Also, you ignore the possibility of providing a replacer. This is perfectly valid: JSON.stringify({ value: 2n }, []). To consider this you would need yet another overload.

@brunocrosier
Copy link
Author

That's a good point re: the replacer.

How about this?

  • No utility types
  • Only updates JSON.parse to disallow arguments we know cannot be parsed

@MartinJohns
Copy link
Contributor

I'm no member of the TypeScript team, so I have no say. But I don't see the point in vastly overcomplicating these types for a negligible use case. This would only work for inferred string literal types, and I can't think of a reason why you would pass a string literal to JSON.parse() instead of having the object literal directly.

Honestly, this would be much better suited as an ESLint rule.

@brunocrosier
Copy link
Author

Yeah, you're right actually. My bad

@brunocrosier brunocrosier closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2022
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 a pull request may close this issue.

5 participants