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

Made additionalProperties add an index signature to unkown when explicit properties are specified #1719

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

filkalny-thimble
Copy link

This allows objects with additional properties to be used without polluting the rest of the codebase with anys.

@mrlubos
Copy link
Collaborator

mrlubos commented Feb 4, 2024

Hey @filkalny-thimble, why do you prefer unknown instead of any?

@filkalny-thimble
Copy link
Author

filkalny-thimble commented Feb 4, 2024

Hey @filkalny-thimble, why do you prefer unknown instead of any?

unknown ensures type safety by not being assignable before having its type narrowed. Consider the following example, which will not cause any TS errors but will crash when ran:

const anyTyped: any = { foo: "bar" }

type MyType = { foo: { faz: string } }
const concreteTyped: MyType = anyTyped; // TS sees no issues with this

// This will fail with "cannot access properly length of undefined"
console.log(concreteTyped.foo.faz.length);

The same example with unknown instead of any will produce a TS error forcing you to do a check at runtime that the unknown variable is of the expected type:

const unknownTyped: unknown = { foo: "bar" }

type MyType = { foo: { faz: string } }
// TS would not be happy with the following:
// const concreteTyped: MyType = unknownTyped;

// so instead you have to narrow the type
const isMyType = (val: unknown): val is MyType => typeof val === "object" && "foo" in val && typeof val.foo === "object" && "faz" in val.foo && typeof val.foo.faz === "string";

if (isMyType(unknownTyped)) {
  const concreteTyped: MyType = unknownTyped;

  // Now we can operate safely
  console.log(concreteTyped.foo.faz.length);
}

@mrlubos
Copy link
Collaborator

mrlubos commented Feb 4, 2024

@filkalny-thimble I see. OpenAPI has a concept of Any Type schema that matches any data type. Would you also expect Any Type schema to result in unknown in TypeScript?

@filkalny-thimble
Copy link
Author

filkalny-thimble commented Feb 4, 2024

@filkalny-thimble I see. OpenAPI has a concept of Any Type schema that matches any data type. Would you also expect Any Type schema to result in unknown in TypeScript?

Yes, this is what I would expect, despite the coincidentally similar naming to typescript's any.
This is because, as the documentation states, AnyType is equivalent to a union of all types (string | number | boolean | ...etc), in other words anything is assignable to it. Both TS any and unknown fulfill this.
However a union of all types is not itself assignable to anything but its own type. any does not satisfy this whereas unknown does.
That is why I would expect AnyType to generate unknown.

@mrlubos
Copy link
Collaborator

mrlubos commented Feb 4, 2024

@filkalny-thimble Great, I was hoping that would be the answer. I think we have two approaches towards handling these cases, any and unknown. I believe I've seen libraries move towards unknown in the past, but I can't recall any examples. Do you think it would make sense to give users an option to choose which value they prefer? Or should the package be opinionated on this?

@filkalny-thimble
Copy link
Author

@filkalny-thimble Great, I was hoping that would be the answer. I think we have two approaches towards handling these cases, any and unknown. I believe I've seen libraries move towards unknown in the past, but I can't recall any examples. Do you think it would make sense to give users an option to choose which value they prefer? Or should the package be opinionated on this?

The only reasonable case I would see for having a user switch built in is so that a breaking change is not introduced into existing code bases. This option should be communicated as such; maybe --legacyAnyType.
Other than that, the choice to make AnyType generate any while not offering such a choice for any of the other OpenAPI types seems arbitrary to me.

@mrlubos
Copy link
Collaborator

mrlubos commented Feb 4, 2024

@filkalny-thimble I see. I might make the switch to unknown as it makes sense. We've got additionalProperties working in our fork, just need to offer the option to make them unknown over any

@mrlubos
Copy link
Collaborator

mrlubos commented Mar 17, 2024

@filkalny-thimble hej, I released v0.27.30 with a bunch of unknown types instead of any. Would you be able to try it and let me know if it works as you expect? Thanks!

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 this pull request may close these issues.

2 participants