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

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

Closed
fnimick opened this issue Aug 27, 2024 · 5 comments
Assignees
Labels
📦 inngest Affects the `inngest` package

Comments

@fnimick
Copy link

fnimick commented Aug 27, 2024

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):

  • npm package Version: 3.22.0

Additional context
Add any other context about the problem here.

@fnimick
Copy link
Author

fnimick commented Aug 27, 2024

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

@jpwilliams jpwilliams self-assigned this Aug 28, 2024
@jpwilliams jpwilliams added Bug Something isn't working 📦 inngest Affects the `inngest` package labels Aug 28, 2024
@jpwilliams
Copy link
Member

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).

@jpwilliams jpwilliams removed the Bug Something isn't working label Sep 2, 2024
@fnimick
Copy link
Author

fnimick commented Sep 4, 2024

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.

@jpwilliams
Copy link
Member

Thanks for the reply!

Closing this as it seems like things are working as intended. I'll note that we would like to add more complex middleware typing at some point to allow you to customize the output of steps (to remove or add to the existing "jsonification").

@jpwilliams jpwilliams closed this as not planned Won't fix, can't repro, duplicate, stale Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 inngest Affects the `inngest` package
Projects
None yet
Development

No branches or pull requests

2 participants