Either.stringifyJSON parameter is too wide

[OliverJAsh]

🐛 Bug report

Current Behavior

Extracted from gcanti/io-ts-types#154 (comment)

It's easy to make mistakes such as:

// You probably meant to remove the undefined values or
// convert them to `null` before calling `stringifyJSON`,
// since JSON cannot contain `undefined` values
E.stringifyJSON({ foo: undefined }, () => '')

or

import * as E from 'fp-ts/Either';
import { pipe } from 'fp-ts/function';

pipe(
    E.stringifyJSON(undefined, () => ''),
    E.fold(
        () => {},
        (v) => {
            // Type is `string`
            // `undefined` at runtime ❗️
            console.log(v);
        },
    ),
);

Expected behavior

Type errors in both of the examples above since they are clearly mistakes.

Reproducible example

See above

Suggested solution(s)

Change the parameter type from unknown to Either.Json.

Additional context

Your environment

Which versions of fp-ts are affected by this issue? Did this work in previous versions of fp-ts?

Software	Version(s)
fp-ts
TypeScript

[gcanti]

Thanks @OliverJAsh

Change the parameter type from unknown to Either.Json.

I can't change the type because would be a breaking change, we can consider this change for v3.0.0 though.

This is actually a bug:

pipe(
  E.stringifyJSON(undefined, () => ''),
  E.map((s) => s.trim()) // throws Cannot read property 'trim' of undefined
)

I didn't expect that JSON.stringify(undefined) = undefined

alas its signature doesn't account for this case so I didn't notice the issue when implementing stringifyJSON

/**
 * Converts a JavaScript value to a JavaScript Object Notation (JSON) string.
 * @param value A JavaScript value, usually an object or array, to be converted.
 * @param replacer A function that transforms the results.
 * @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
 */
stringify(value: any, replacer?: (this: any, key: string, value: any) => any, space?: string | number): string;
/**
 * Converts a JavaScript value to a JavaScript Object Notation (JSON) string.
 * @param value A JavaScript value, usually an object or array, to be converted.
 * @param replacer An array of strings and numbers that acts as an approved list for selecting the object properties that will be stringified.
 * @param space Adds indentation, white space, and line break characters to the return-value JSON text to make it easier to read.
 */
stringify(value: any, replacer?: (number | string)[] | null, space?: string | number): string;

Any ideas how to fix this?

[gcanti]

we can consider this change for v3.0.0 though

another option is adding a new Json module and deprecate the Json-related APIs in Either.ts

[OliverJAsh]

Any ideas how to fix this?

There's no way to fix it other than wrapping JSON.stringify to fix the types. Hopefully TS will fix this soon. Related: microsoft/TypeScript#18879.

[gcanti]

what about

export const stringifyJSON = <E>(u: unknown, onError: (reason: unknown) => E): Either<E, string> =>
  tryCatch(() => {
    const s = JSON.stringify(u)
    if (typeof s !== 'string') {
      throw new Error('undefined, Functions, and Symbols are not valid JSON values')
    }
    return s
  }, onError)

[OliverJAsh]

Is that a temporary solution until we can narrow the parameter type from unknown to Json in v3? If so then yeah that would be great.

[gcanti]

@OliverJAsh yeah, I would like to fix the existing bug asap. As a long term solution I would add a new Json module

[gcanti]

Change the parameter type from unknown to Either.Json.

@OliverJAsh I'm concerned about the usability of such an API:

import { Either } from 'fp-ts/Either'

type Json = boolean | number | string | null | JsonArray | JsonRecord

interface JsonRecord {
  readonly [key: string]: Json
}

interface JsonArray extends ReadonlyArray<Json> {}

declare const stringify: (json: Json) => Either<unknown, string>

interface Person {
  readonly name: string
  readonly age: number
}

declare const person: Person
declare const persons: ReadonlyArray<Person>

stringify(person) // doesn't compile: Index signature is missing in type 'Person'
stringify(persons) // doesn't compile

[OliverJAsh]

That's because of a TS limitation with interfaces: microsoft/TypeScript#15300.

If you switch Person to a type then it works as expected:

type Person = {
    readonly name: string;
    readonly age: number;
}

If you want to use interfaces then you have to declare an index signature:

 interface Person {
+    readonly [key: string]: string | number;
     readonly name: string;
     readonly age: number;
 }

IIUC, the reason TS does not infer an index signature for interfaces like it does for types is because interfaces can be extended in other files, meaning you could add other properties with different types, whereas types are sealed at the point you define them. microsoft/TypeScript#15300 (comment)

[gcanti]

@OliverJAsh TIL, thanks for the link.

We can't force users to use type instead of interface, so I think we should keep unknown.

You probably meant to remove the undefined values or convert them to null before calling stringifyJSON, since JSON cannot contain undefined values

Probably. But I'm not sure we can assume this, if intentional this is valid use case (JSON.stringify will strip undefined props):

console.log(JSON.stringify({ a: 'a', b: undefined })) // => '{"a":"a"}'

[OliverJAsh]

I feel like it's acceptable to ask users who really want to use interfaces to add an explicit index signature so that TypeScript can provide guarantees, until the issue I mentioned is fixed.

Another workaround:

stringify({ ...person });

It's a trade off.

Probably. But I'm not sure we can assume this, if intentional this is valid use case (JSON.stringify will strip undefined props):

You could still narrow the parameter type from unknown to E.Json but add an explicit undefined to support that use case, if that's what you wanted.

import { Either } from 'fp-ts/Either';

type Json = boolean | number | string | null | JsonArray | JsonRecord;

interface JsonRecord {
    readonly [key: string]: Json;
}

interface JsonArray extends ReadonlyArray<Json> {}

type JsonInput = Json | { [key: string]: JsonInput | undefined };

declare const stringify: (json: JsonInput) => Either<unknown, string>;

console.log(stringify({ a: 'a', b: undefined })); // => '{"a":"a"}'
console.log(stringify({ a: 'a', b: undefined, c: () => {} })); // errors as expected ✅

[gcanti]

I feel like it's acceptable to ask users who really want to use interfaces to add an explicit index signature

mmhh I disagree, personally I'm not going to change my domain model just to please stringify

Another workaround:

Nice

You could still narrow the parameter type from unknown to E.Json but add an explicit undefined to support that use case, if that's what you wanted.

A strict Json type is useful when it appears on the output (the parse function).

Also there are other valid use cases other than undefined (and the undefined could be in a nested structure)

// a function
console.log(JSON.stringify({ a: 'a', b: () => {} })) // => '{"a":"a"}'
// a nested undefined
console.log(JSON.stringify({ a: 'a', b: { c: 'c', d: undefined } })) // => '{"a":"a","b":{"c":"c"}}'

Btw I must add a type parameter in order to fix another issue (I'm using the new Json module here):

import * as E from 'fp-ts/Either'
import { pipe } from 'fp-ts/function'
import * as _ from 'fp-ts/Json'

// returns `Either<unknown, unknown>`, should be `Either<unknown, string>`
pipe(E.right('a'), E.chainFirst(_.stringify))

changing the signature to declare export const stringify: <A>(a: A) => Either<unknown, string> fixes the issue.

You could leverage the new type parameter to restrict the type of a to Json in your specific use case

// $ExpectError
_.stringify<_.Json>({ a: undefined })
// $ExpectError
_.stringify<_.Json>({ ...{ a: undefined } })

[OliverJAsh]

Should I close this given we're not going with my original suggestion, or do you want me to leave it open to track the other discussion points?

[gcanti]

Let's keep this open until 2.10 is merged into master

Here's the new Json module

[enricopolanski]

Just my 2 cents, but as long as we rely on JSON.stringify or parse we'll always be exposed to odd behavior, and worse, runtime errors.

This is because in order to be converted JavaScript relies on the toString and valueOf methods on the prototype chain, which can (and sometimes are) overwritten exposing our code to (runtime) errors.

My opinion is that if we want to rely on those two functions they should be wrapped in a tryCatch, since my understanding is that the intention is already to return an Either this should not be a breaking change.