inngest / inngest-js

The developer platform for easily building reliable workflows with zero infrastructure for TypeScript & JavaScript
https://www.inngest.com/
GNU General Public License v3.0
392 stars 40 forks source link

[BUG] `Jsonify` marks properties that are required and may be undefined as optional #689

Open fnimick opened 2 weeks ago

fnimick commented 2 weeks ago

Describe the bug Jsonify marks all properties that may be undefined as optional, which breaks when dealing with types that have required, potentially undefined properties.

To Reproduce

Example:

import { type Jsonify } from "inngest/helpers/jsonify";

interface Test {
  prop: string | null | undefined;
  requiredProp: string;
  optionalProp?: string | null;
}

type TestJsonify = Jsonify<Test>;

TestJsonify has the following type:

type TestJsonify = {
    prop?: string | null | undefined;
    requiredProp: string;
    optionalProp?: (string | null) | undefined;
}

This means that values of type Jsonify<Test> can no longer be assigned to values of type Test, as the required, potentially undefined property no longer type checks.

Expected behavior The type of Jsonify<object> and the object itself should match, for simple interfaces like this.

Code snippets / Logs / Screenshots If applicable, add screenshots to help explain your problem.

System info (please complete the following information):

Additional context Add any other context about the problem here.

linear[bot] commented 2 weeks ago

INN-3576 [BUG] `Jsonify` marks properties that are required and may be undefined as optional

fnimick commented 2 weeks ago

This is similar to #534 - but occurs when strictNullChecks is enabled.

jpwilliams commented 1 week ago

Thanks for the report, @fnimick!

The tricky thing here is that JSON has no concept of undefined; while a JS value can be undefined, the only way to represent undefined in JSON is by omitting the key entirely. Therefore, making the key optional (in order to represent undefined) is expected.

A workaround in situations like this could be a type assertion: jsonResult as Test, but depending on the precise context this could be fragile.

Given the inability for JSON to represent undefined, what would your expected behaviour be here? A future alternative would be the ability to provide your own transformers for data (e.g. superjson, EJSON, custom).

fnimick commented 1 week ago

Ah, I didn't realize that. I think explicitly typing (edit: as a user) our interface types to allow for undefined / optional is the best, when those interfaces are going to be returned from inngest calls. (having different behavior for those cases is a HUGE code smell anyway). So in that way, the types are correct, and flagging the type error here is the right thing to do!

I'm not sure how this interacts with the exactOptionalPropertyTypes typescript option either - thought it's pretty rare for that to be used IME, most libraries are not built with that in mind and it causes TONS of errors for interfaces that don't specify optional properties as being allowed to be present but explicitly undefined.