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

Emitting optional fields in generated TypeScript #100

Closed
lostfictions opened this issue Jan 2, 2023 · 3 comments
Closed

Emitting optional fields in generated TypeScript #100

lostfictions opened this issue Jan 2, 2023 · 3 comments

Comments

@lostfictions
Copy link

lostfictions commented Jan 2, 2023

Hi there, thanks so much for this library! I've been trying it out for a few days in a Tauri project and it's been a real pleasure to use so far.

One small point of friction I've encountered is when I have procedures that take complex inputs with a number of optional fields. Let's say I have an query like this:

let router = Router::<Arc<prisma::PrismaClient>>::new()
    .query("search", |t| {
        #[derive(Type, Deserialize)]
        enum OrderBy {
            Relevance,
            Time,
        }

        #[derive(Type, Deserialize)]
        pub struct SearchInput {
            query: String,
            snippet_token_limit: Option<u32>,
            count: Option<u32>,
            offset: Option<u32>,
            order_by: Option<OrderBy>,
            // ...
        }

        t(|db, input: SearchInput| async move { /* ... */ })
    )
    .build()
    .arced();

That results in emitted TypeScript that looks something like this:

export type Procedures = {
  queries: { key: "search"; input: SearchInput; result: Array<SearchResult> };
  mutations: never;
  subscriptions: never;
};

export type OrderBy = "Relevance" | "Time";

export interface SearchInput {
  query: string;
  snippet_token_limit: number | null;
  count: number | null;
  offset: number | null;
  order_by: OrderBy | null;
  // ...
}

On the frontend I can then consume it like this:

  const searchQuery = useQuery([
    "search",
    {
      query: "cool search terms here",
      snippet_token_limit: null,
      count: null,
      offset: null,
      order_by: null,
      // ...
    },
  ]);

This works, but it's a little verbose. I'd like to do something like this:

const searchQuery = useQuery(["search", { query: "an even cooler search" }]);

Much better! And this is much more idiomatic for JavaScript and TypeScript.

I think it would be nice if Specta could support this by adding a ? prefix (or | undefined) to the type annotation of optional fields. This could maybe be configurable via a field or struct attribute... but honestly I'm not sure it needs to be.

Generally the most idiomatic way to express optionality in TypeScript is like this:

export interface SearchInput {
  query: string;
  snippet_token_limit?: number;
  count?: number;
  offset?: number;
  order_by?: OrderBy;
  // ...
}

It could also be fine to allow both null and undefined:

export interface SearchInput {
  query: string;
  snippet_token_limit?: number | null;
  count?: number | null;
  offset?: number | null;
  order_by?: OrderBy | null;
  // ...
}

I think that fieldname?: T | null is also the same as fieldname: T | null | undefined, but there might be nuance -- see exactOptionalPropertyTypes.

For what it's worth, Serde (and thus rspc) already support deserializing undefined into None for an Option<T>. So in the above example, if I simply omit the nullable fields instead of explicitly setting them to null (ignoring the TypeScript errors that yields at the moment), everything works fine -- it's just that the emitted TypeScript doesn't like that.

@Brendonovich
Copy link
Collaborator

Brendonovich commented Jan 3, 2023

Specta actually supports the #[speta(optional)] field attribute for just this use case, it'll export fields as field?: T instead of field: T | null. Unless I messed something up in the implementation, you should be good to go.

btw field?: T | null is different to field: T | null | undefined. The former allows field to not be defined on the object, whereas the latter requires that field to always have a value

@lostfictions
Copy link
Author

Thanks, #[specta(optional)] works! Would love to see that become the default behaviour since it's so much more idiomatic for TypeScript... is there a reason it shouldn't be?

@Brendonovich
Copy link
Collaborator

In my view we do it for two reasons:

  1. It's the approach most inline with serde: While undefined, null, and a lack of a value entirely can be deserialized to Option, the default serialization of None will always be null. This can be changed with things like skip_serializing_if = "Option::is_none" (which Specta will be able to detect at some point), but the default behaviour being None == null will always hold.
  2. While they're commonly used interchangeably, null and undefined are very different and useful to distinguish in certain situations. It's important that we don't just conflate the two as a blanket solution.

There's been some discussion about this over in ts-rs, but even that isn't really beneficial for your use case since it's only discussing serialisation. Specta treats all types as if they're going to be both serialised and deserialised, so it sticks to the lowest common type denominator between the two. Maybe we should be a hint that tells Specta if you're only ever using one, like #[specta(no_serialize)], in which case it would output field?: null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants