elysiajs / elysia

Ergonomic Framework for Humans
https://elysiajs.com
MIT License
10.12k stars 215 forks source link

Returning non-JSON serializable data types results in messed up typing #262

Closed ptrxyz closed 1 month ago

ptrxyz commented 12 months ago

If non-JSON serializable data types are returned by a route (i.e. Date, Set, undefined, etc), the client would not detect this. Eden fetch/treaty would both happily claim that the returned data is of type Set/Date/etc yet they are not. This will break the program eventually, as soon as you try to access properties or call methods on them.

I would suggest to allow users to plug in more potent transformers (such as superjson or devalue) or to make the client "know" about the broken types by returning never or unknown or any other fallback type.

But letting the user think the API would be type-safety while it is only to some extend is dangerous imo.

ptrxyz commented 12 months ago

I this also the case for Eden Fn btw? Would it also detect types wrongly?

ptrxyz commented 12 months ago

Seems related: https://github.com/elysiajs/elysia/issues/175

sirchnik commented 4 months ago

I use this to serialize non-JSON types. It works for me temporarily but a few ifs are missing.

export const elysiaApp = new Elysia()
    .onAfterHandle(({ response }) => {
        if (response && typeof response === 'object') {
            if (ELYSIA_RESPONSE in response) {
                return;
            }
            return new Response(SuperJSON.stringify(response));
        }
    });
export const eden = treaty<ElysiaApp>('localhost:3334', {
    fetch: {
        credentials: 'include',
    },
    async onResponse(response) {
        return SuperJSON.parse(await response.text());
    },
});
SaltyAom commented 1 month ago

Closing as not going to implement in the core of Elysia.

You can easily create a adapter for Superjson natively as describe here.

I use this to serialize non-JSON types. It works for me temporarily but a few ifs are missing.

export const elysiaApp = new Elysia()
  .onAfterHandle(({ response }) => {
      if (response && typeof response === 'object') {
          if (ELYSIA_RESPONSE in response) {
              return;
          }
          return new Response(SuperJSON.stringify(response));
      }
  });
export const eden = treaty<ElysiaApp>('localhost:3334', {
  fetch: {
      credentials: 'include',
  },
  async onResponse(response) {
      return SuperJSON.parse(await response.text());
  },
});
EvHaus commented 1 month ago

@SaltyAom Thanks for responding. If this isn't implemented in Elysia itself, isn't it somewhat of a bug though?

Consider this code:

import { Elysia } from 'elysia';
import { treaty } from '@elysiajs/eden'

const elysiaApp = new Elysia()
    .get('/foo', () => {
        return {
            // Let's return a Date object from our handler...
            bar: new Date()
        }
    })
    .listen(3000);

const app = treaty<typeof elysiaApp>('localhost:3000');

// TypeScript types this as {data: {bar: Date}}
const {data} = await app.foo.get();

// But when you actually look at the type, this prints `string`
console.log(typeof data?.bar);

Elysia is doing the right thing and coverting Date objects to strings already, but it doesn't set the right type for them. Adding superjson feels a bit like overkill to me, as it's going to add a layer of complexity (+ a performance hit) just to fix the wrong type being returned.

Personally, the solution I've been using myself is to assert the type in the handler, ie.

.get('/foo', () => {
    return {
        // Ugly type assertion here
        bar: new Date() as unknown as string
    }
})

Which is messy, but at least it allows the rest of the code downstream to have the correct type set.

kravetsone commented 1 month ago

@SaltyAom Thanks for responding. If this isn't implemented in Elysia itself, isn't it somewhat of a bug though?

Consider this code:

import { Elysia } from 'elysia';
import { treaty } from '@elysiajs/eden'

const elysiaApp = new Elysia()
  .get('/foo', () => {
        return {
            // Let's return a Date object from our handler...
            bar: new Date()
        }
    })
    .listen(3000);

const app = treaty<typeof elysiaApp>('localhost:3000');

// TypeScript types this as {data: {bar: Date}}
const {data} = await app.foo.get();

// But when you actually look at the type, this prints `string`
console.log(typeof data?.bar);

Elysia is doing the right thing and coverting Date objects to strings already, but it doesn't set the right type for them. Adding superjson feels a bit like overkill to me, as it's going to add a layer of complexity (+ a performance hit) just to fix the wrong type being returned.

Personally, the solution I've been using myself is to assert the type in the handler, ie.

.get('/foo', () => {
    return {
        // Ugly type assertion here
        bar: new Date() as unknown as string
    }
})

Which is messy, but at least it allows the rest of the code downstream to have the correct type set.

Just add response schema

EvHaus commented 1 month ago

Just add response schema

True, that would address this as well. I still think the current behavior is a bug. The types do not match the actual data being returned. Having to add type assertions, or a response schema feels like a workaround, rather than a conscious design choice.

kravetsone commented 1 month ago

Just add response schema

True, that would address this as well. I still think the current behavior is a bug. The types do not match the actual data being returned. Having to add type assertions, or a response schema feels like a workaround, rather than a conscious design choice.

it is really complex if elysia tries to remap non-json serializable data types i guess types will be more slower