brillout / wildcard-api

Functions as API.
MIT License
368 stars 14 forks source link

[typescript] How to type the context? #51

Closed michie1 closed 4 years ago

michie1 commented 4 years ago

In the authentication example it's possible to extend the context, but this is not typed when noImplicitThis is set to true. 'this' implicitly has type 'any' because it does not have a type annotation "noImplicitThis".

It would be nice if this was automatically being done by using the type of the getContext function.

"Solutions":

brillout commented 4 years ago

How about this:

type Context = {
  userId: number;
};

function getContext(): Context {
   /* ... */
}
endpoints.hello = async function(){
   return "Your user ID is: "+(this as Context).userId;
}
michie1 commented 4 years ago

Sure, that works. This yields 'this' implicitly has type 'any' because it does not have a type annotation. Although not ideal because it's needs to be done for every usage. For me this is fine for now, but I think it would be a good discussion to improve this for a new api.

https://github.com/reframejs/wildcard-api/issues/39#issuecomment-599043728 Here you suggested to have a public setPrivateKey which can modify the context. Maybe we can have a public function getContext() as well that is typed.

brillout commented 4 years ago

Although not ideal because it's needs to be done for every usage

Yea I know. A way to somewhat alleviate the problem is to wrap the most common usages in functions.

endpoints.hello = async function(){
   const user = retrieveUser(this);
   return "You are: " + user.name;
}

function retrieveUser(this as Context) {
   /*...*/
}

I agree that this not ergonomic and let me know when this becomes too cumbersome.

I think it would be a good discussion to improve this for a new api.

I agree.

39 (comment) Here you suggested to have a public setPrivateKey which can modify the context. Maybe we can have a public function getContext() as well that is typed.

Yes, I believe there is a way to make auth/context super simple.

michie1 commented 4 years ago

A way to somewhat alleviate the problem is to wrap the most common usages in functions.

endpoints.hello = async function(){
   const user = retrieveUser(this);
   return "You are: " + user.name;
}

function retrieveUser(this as Context) { // You mean "this: Context" probably, or it is a syntax I have not seen yet before
   /*...*/
}

In Javascript this would be nice, but in Typescript it will be an issue. This example will yield two errors: 1) retrieveUser(this) yields Expected 0 arguments, but got 1.. This could be solved by using retrieveUser.call(this), although a bit cumbersome. 2) retrieveUser(this) yields also 'this' implicitly has type 'any' because it does not have a type annotation., which is also the error of your endpoints.hello example.

brillout commented 4 years ago

I´ll eventually add TypeScript to the source code and I'll then look into typing the Context.

brillout commented 4 years ago

I added TS types to the source code.

My latest attempt: https://github.com/reframejs/wildcard-api/commit/71e85f7ad173280abe46fd4850ef2dfbdba93ad2

I guess the goal here would be able to create a type that is both Server and EndpointsWithContext. But I'm not sure how to achieve that, I'm fairly new to TS.

(Note that endpoints are now named server, accommodating realtime bidrectional communication.)

michie1 commented 4 years ago

I solved it locally by using a helper function:

function context(that: Context) {
  return that;
}

and call it with context(this).user for example. I don't know how to change the library so we can omit this helper function.

brillout commented 4 years ago

Neat that's clever.

I don't know how to change the library so we can omit this helper function.

Yes I'm thinking a lot about this but I can't find a solution so far.

The problem with something like this:

// The `server` object is a proxy around your context object.
// The `michieEndpoint` function is binded to `server`
server.michieEndpoint = function(greeting='Hello'): string {
  // TS knows that `this.user` exists!
  return greeting+' '+this.user;
}

is how do you now export the type of michieEndpoint? The following doesn't work:

server.michieEndpoint = function () {};

// `michieEndpoint` is *dynamically* defined on the `server` object.
// TS doesn't know that `server` has the property `michieEndpoint`.
// How can we export the type of `server.michieEndpoint`?

// TS doesn't know that `Endpoints` has the property `michieEndpoint`.
export type Endpoints = typeof server;

Doing something like this doesn't work in JS:

const server = {};
server.michieEndpoint = function michieEndpoint() {};
// JS: `michieEndpoint is not defined`
console.log(michieEndpoint);

which, I guess, would otherwise lead to some kind of non-determinism.

So it boils down to either:

michie1 commented 4 years ago

Just some brainstorming, I'm thinking how to omit using this:

A: Could it be possible to export a function getContext() from the library which can be used in the function declaration? On the server we might need to call const wc = wildcard.init<Context>(); When the function is called on the client side it's connected to request (e.g. Express) middle ware.

B: On the frontend you call a function for example with foo(5), but on the backend you can define the function as foo(context: Context, value: number). On the frontend you could use a wrapper type that omits the first parameter.

type Context = {
  foo: number;
};

function foo(context: Context, a: number) {
  return a + context.foo;
}

type FooType = typeof foo;
type FooMinusContext = FooType extends (context: Context, ...rest: infer U) => infer R
  ? (...rest: U) => R
  : never;

https://github.com/reframejs/wildcard-api/blob/3ed87113503cb75d09a7cb4f5669a7ba9fe26371/server/WildcardServer.ts#L287

Here you could inject the context as first argument. Potentially you could place it as last argument and keep using the thisArgument of the apply function, so Javascript users can keep using it the current way, while Typescript users could use the correctly typed version.

brillout commented 4 years ago

A: Imagine two requests to michieEndpoint. Now imagine michieEndpoint is asynchronous — there are two Express req objects; how do you know which to provide to each of the two getContext call? I'm not sure this is possible.

B: Interesting... I was focused on this because I kinda liked it but you're right it doesn't have to be this, which would seem to make things easier.

type FooType = typeof foo;
type FooMinusContext = FooType extends (context: Context, ...rest: infer U) => infer R
  ? (...rest: U) => R
  : never;

That's a neat construct, I wasn't aware of infer.

C: Maybe another possibility; extend the global object to add the context to the global this. Similar to how the browser's window and Node.js's global is usually being typed. But I'm afraid of the side-effect of "polluting" the global this for non-endpoint functions.

Seems like B is the most promising option so far.

cc @lostpebble, who was actually the one who found the typeof trick.

michie1 commented 4 years ago

I was re-reading the issue and saw one of my first "solutions".

* Typing the `this` parameter. See https://www.typescriptlang.org/docs/handbook/functions.html#this-parameters This solution won't work well, because now the frontend also expects an extra parameter.

This solution in combination with FooMinusContext applied to each function would work as well I think. I'll try to make that type. If it works correctly the only thing the library needs to do is exporting that type. It won't be a breaking change.

rjkz808 commented 3 years ago

B: Interesting... I was focused on this because I kinda liked it but you're right it doesn't have to be this, which would seem to make things easier.

type FooType = typeof foo;
type FooMinusContext = FooType extends (context: Context, ...rest: infer U) => infer R
  ? (...rest: U) => R
  : never;

That's a neat construct, I wasn't aware of infer. ... Seems like B is the most promising option so far.

wildcard-api is awesome but just wanted to clarify - is plan B still relevant? IMO it's better to avoid JS dynamic this whenever it's possible, so it seems to me that B is actually a good solution. @brillout what do you think about it?

rjkz808 commented 3 years ago

btw it'll also resolve issues related to arrow functions, there's a lot of devs who prefer arrows over function declarations.

brillout commented 3 years ago

@rjkz808 Yes, there is a new version major coming.

import { server, useContext } from 'telefunc/server' // Wildcard API will be renamed Telefunc

server.hello = () => {
  const context = useContext()
  return `Hello ${context.user?.name || 'Anonymous'}`
}

Telefunc doesn't return a type for useContext() so that the user can type it globally once.

rjkz808 commented 3 years ago

@brillout that's nice, but as I see by the semantics of useContext, it'll be required to call it at the top level of the function before any asynchronous operations to prevent using the context of another request. Am I correct?

brillout commented 3 years ago

That is correct. (Although the current implementation uses async hooks so that the user can use useContext() everywhere, but this needs to be optional because not every platform supports async hooks, e.g. CF workers and Deno.)

Do you have alternative ideas in mind?

brillout commented 3 years ago

Solution B requires the user to transform the type between the (tele)function server-side definition and the (tele)function client-side consumption, whereas with useContext server-side and client-side type definition are the same which I find quite neat.