brillout / telefunc

Remote Functions. Instead of API.
https://telefunc.com
MIT License
690 stars 31 forks source link

Config option for disabling shield errors on obsolete fields #81

Open samuba opened 1 year ago

samuba commented 1 year ago

Problem

Would solve https://github.com/brillout/telefunc/issues/78. The shields for telefunc's will throw validation errors when request parameters have fields defined that are not in the type definition. What makes this behaviour problematic is the fact that typescript is structurally typed, meaning it won't help you to find these bugs in the IDE. (if there is at least 1 level of indirection) See Example:

interface User { name: string }

function myTelefunc(user: User) { 
  // implementation
}

// typescript shows error: Object literal may only specify known properties, and 'bar' does not exist in type 'User'
// shield throws validation error for this
myTelefunc({ name: "foo", bar: 1})

// typescript will not show any error for this
// shield throws validation error for this
const userObj = { name: "foo", bar: 1}
myTelefunc(userObj)

See TS playground.

Solution

I propose a new config option which makes the shields not throw validation errors for objects containing fields which are not part of the type definition. This would actually align the shields type validation behaviour with typescripts validation behaviour.

brillout commented 1 year ago

Is there a TypeScript config to make this invalid?

const userObj = { name: "foo", bar: 1}
myTelefunc(userObj)

I wonder what TypeScript's rationale is for differentiating this from myTelefunc({ name: "foo", bar: 1}).

As for Telefunc, how can Telefunc tell the difference?

samuba commented 1 year ago

I'm pretty sure that there is no config option to make this invalid. This behaviour was a conscious decision in the language design of typescript and is fundamental to a lot of the stuff build upon it. In general typescript types behave more like interfaces from languages like Go.

The big surprise to me was that passing an object literal with an obsolete field is invalid. Maybe their rational was: If you are so explicit in passing it as a literal you most probably want to fully adhere to the type definition. Still feels weird to me though. I consider this an edge case as most of the objects you pass into a function are not literals. Thats why I think there is not much value for telefunc in finding out if it was passed as literal or not. I'd say, let's optimize for 90% of the cases and make the shields not throw validation errors on objects with obsolete fields, if the config option is set.

brillout commented 1 year ago

Makes sense.

The usecase was reading users config from localstorage and then sending it to a telefunc. Some months ago I removed a field from the config and only today found out that apparently some users still have that field in their localstorage, because those requests fail for them.

To be honest, it seems quite an edge case. So I'm leaning towards making this lower priority.

samuba commented 1 year ago

I agree that usecase is edge case'y but my proposed solution would help for all other cases also. My other three bugs had no localstorage involved just straight parameter passing and for these I also wished I had the proposed config option.

samuba commented 1 year ago

I don't know if I find the time to do this but would you be open for a PR on this?

brillout commented 1 year ago

Yes, I can see it to be fairly simple. (If it ends up being complex, keep in mind that I may reject if it's too time consuming for me to review.)