Fix typing of `json` runtype output #73

remnantkevin · 2022-08-07T02:43:33Z

Reproduction of issue: codesandbox.io

While using the json runtype for the first time lately, I noticed that, while the runtype was working in terms of producing the correct output, it was losing type information. For example, after adding the tests from this PR, but before changing anything in src/json.ts, if I ran yarn test:types I got the following output:

output

❯ yarn run test:types
yarn run v1.22.19
$ tsd

  test-d/json.test-d.ts:11:21
  ✖  11:21  Argument of type unknown is not assignable to parameter of type number.                                                                                                                                                                       
  ✖  16:21  Argument of type unknown is not assignable to parameter of type string.                                                                                                                                                                       
  ✖  21:23  Argument of type unknown is not assignable to parameter of type number[].                                                                                                                                                                     
  ✖  26:28  Argument of type unknown is not assignable to parameter of type { a: string; }.                                                                                                                                                               

  test-d/record.test-d.ts:51:57
  ✖  51:57  Argument of type { a: string; b: unknown; d: string; } is not assignable to parameter of type { a: string; b: { c: string; }; d: string; }.
  Types of property b are incompatible.
    Type unknown is not assignable to type { c: string; }.  

  5 errors

error Command failed with exit code 1.

And in VS Code I can see the following in the json tests that already exist in test/json.test.ts:

screenshot

After making this PR's changes in src/json.ts, the tests pass and I see the following in VS Code:

screenshot

remnantkevin · 2022-08-07T03:18:25Z

test-d/json.test-d.ts

+// basic
+{


Added these. Not sure if there should be more / if there are other tests that would be preferred.

Looks good to me.

remnantkevin · 2022-08-07T03:22:10Z

test-d/record.test-d.ts

+// record with json
+{
+  const rt = st.record({
+    a: st.string(),
+    b: st.json(
+      st.record({
+        c: st.string(),
+      }),
+    ),
+    d: st.string(),
+  })
+
+  expectType<{ a: string; b: { c: string }; d: string }>(rt(data))
+}


Because the tests in test-d/json.test-d.ts check that json produces the correct types, this test is not strictly necessary, so I can remove if that is preferred. I also know it's impractical to have tests for all the possible interactions between runtypes. However, I added this one as it is used in my use case (e.g. a record that contains a json, which then contains a record), but I can remove if that is preferred.

Perfect, I completely agree here.

It is not possible to model all interactions but having some tested interactions, especially based on previous bugs is really useful and adds documentation and robustness to the tests.

remnantkevin · 2022-08-07T03:30:20Z

src/json.ts

+export function json<T>(rt: Runtype<T>): Runtype<T> {
+  return internalRuntype<any>((v, failOrThrow) => {


This is the change made to the json runtype.

We want json to return a Runtype which is the same Runtype as the one passed to json. (And this runtype will be applied to the string given to it after the string has been parsed into JSON).

The current change allows for that: the type of the param of json is the same as its return type, and the any generic param given to internalRuntype means that internalRuntype returns a Runtype<any> which can be assigned to Runtype<T>.

I've seen this kind of use of any with internalRuntype in, for example, array and tuple. However, I'm not sure if there is a better way of achieving this without using any, or without explicitly specifying internalRuntype's generic param.

I'm no TS expert, and I'm not sure of all the consequences of this change, so please let me know if this is incorrect 🙂

The way you did is is 100% correct within simple-runtypes.

The reason you need to use any is that I was being lazy with building the "internal runtype" stuff. There are Runtype<T>s and InternalRuntype<T>s. The internal ones have that additional parameter so that they do not throw an error but instead return an error object. This was all done to stay fast (catching errors and using many nested try-catch statements is slow as hell).

But the internalRuntype constructor does return a Runtype,not an InternalRuntype. And also expects a Runtype to be passed through not an InternalRuntype. Internal and external runtypes and constructors should be properly separated and a simple casting type should turn internal into external runtype (that was the idea behind the failOrThrow flag - to not wrap every runtype in another function).

Anyways, as much as I love having everything typed, IMHO it is okay to sometimes have some untyped internals.

hoeck

Thx!

hoeck · 2022-08-18T08:39:22Z

test-d/record.test-d.ts

+// record with json
+{
+  const rt = st.record({
+    a: st.string(),
+    b: st.json(
+      st.record({
+        c: st.string(),
+      }),
+    ),
+    d: st.string(),
+  })
+
+  expectType<{ a: string; b: { c: string }; d: string }>(rt(data))
+}


Perfect, I completely agree here.

It is not possible to model all interactions but having some tested interactions, especially based on previous bugs is really useful and adds documentation and robustness to the tests.

hoeck · 2022-08-18T09:02:06Z

src/json.ts

+export function json<T>(rt: Runtype<T>): Runtype<T> {
+  return internalRuntype<any>((v, failOrThrow) => {


The way you did is is 100% correct within simple-runtypes.

The reason you need to use any is that I was being lazy with building the "internal runtype" stuff. There are Runtype<T>s and InternalRuntype<T>s. The internal ones have that additional parameter so that they do not throw an error but instead return an error object. This was all done to stay fast (catching errors and using many nested try-catch statements is slow as hell).

But the internalRuntype constructor does return a Runtype,not an InternalRuntype. And also expects a Runtype to be passed through not an InternalRuntype. Internal and external runtypes and constructors should be properly separated and a simple casting type should turn internal into external runtype (that was the idea behind the failOrThrow flag - to not wrap every runtype in another function).

Anyways, as much as I love having everything typed, IMHO it is okay to sometimes have some untyped internals.

hoeck · 2022-08-18T09:02:33Z

test-d/json.test-d.ts

+// basic
+{


Looks good to me.

remnantkevin added 3 commits August 6, 2022 15:31

Add failing json type tests

349ae0c

Add failing record with json type test

b5f5296

Fix json runtype type inference

65d2354

remnantkevin changed the title ~~Fix json runtype type inference~~ Fix typing of json runtype output Aug 7, 2022

remnantkevin commented Aug 7, 2022

View reviewed changes

hoeck approved these changes Aug 18, 2022

View reviewed changes

hoeck merged commit 2a141b3 into hoeck:master Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix typing of `json` runtype output #73

Fix typing of `json` runtype output #73

remnantkevin commented Aug 7, 2022 •

edited

Loading

remnantkevin Aug 7, 2022

hoeck Aug 18, 2022

remnantkevin Aug 7, 2022 •

edited

Loading

hoeck Aug 18, 2022

remnantkevin Aug 7, 2022 •

edited

Loading

hoeck Aug 18, 2022

hoeck left a comment

hoeck Aug 18, 2022

hoeck Aug 18, 2022

hoeck Aug 18, 2022

		export function json<T>(rt: Runtype<T>): Runtype<T> {
		return internalRuntype<any>((v, failOrThrow) => {

Fix typing of json runtype output #73

Fix typing of json runtype output #73

Conversation

remnantkevin commented Aug 7, 2022 • edited Loading

remnantkevin Aug 7, 2022

Choose a reason for hiding this comment

hoeck Aug 18, 2022

Choose a reason for hiding this comment

remnantkevin Aug 7, 2022 • edited Loading

Choose a reason for hiding this comment

hoeck Aug 18, 2022

Choose a reason for hiding this comment

remnantkevin Aug 7, 2022 • edited Loading

Choose a reason for hiding this comment

hoeck Aug 18, 2022

Choose a reason for hiding this comment

hoeck left a comment

Choose a reason for hiding this comment

hoeck Aug 18, 2022

Choose a reason for hiding this comment

hoeck Aug 18, 2022

Choose a reason for hiding this comment

hoeck Aug 18, 2022

Choose a reason for hiding this comment

Fix typing of `json` runtype output #73

Fix typing of `json` runtype output #73

remnantkevin commented Aug 7, 2022 •

edited

Loading

remnantkevin Aug 7, 2022 •

edited

Loading

remnantkevin Aug 7, 2022 •

edited

Loading