search

wasp-lang / wasp

The fastest way to develop full-stack web apps with React & Node.js.
https://wasp-lang.dev
MIT License
12.92k stars 1.15k forks source link

Rethink server-side Operations API #1909

Closed infomiho closed 1 month ago

infomiho commented 4 months ago

Update by @sodic

When attempting to fix this, I realized our server-side Operations API is virtually unusable:

This happened because we created this API as an afterthought and plugged users into an arbitrary point of our internal RPC architecture. Therefore, the API has never really been user friendly. To make things worse, since it's basically a hook into our internal implementation, it can easily change without us noticing (e.g., it happened with AuthUser).

This problem slipped under the radar for so long because calling operations from the server isn't that crucial of a feature, but we should still fix it.

Idea: create a separate wrapper for server side queries that's as clean as possible and does most of the ugly work under the hood.

Fixing this will also fix part of #1958.

Original issue

Currently we say

To use a Query, you can import it from wasp/client/operations and call it directly. As mentioned, the usage doesn't change depending on whether you're on the server or the client:

and

To use an Action, you can import it from wasp/client/operations and call it directly. As mentioned, the usage doesn't change depending on whether you're on the server or the client:

which is incorrect. The users shouldn't import the actions and queries on the server from wasp/client/operation but rather from wasp/server/operations.

Also, we need to explain that they need to provide the context as the first argument and then the arguments as the second argument.

Discord thread about this: https://ptb.discord.com/channels/686873244791210014/1218567315185209464/1219591538942804019

Martinsos commented 4 months ago

They can also import action directly, from its file. In which case they have to construct the context completely on their own, while allegedly, if they import it via wasp/server/operations, they don't have to construct it completely on their own, but need to pass just user to the context and rest will be added. We should confirm this and decide if we want to document "raw" import approach also or not.

leoschet commented 4 months ago

@Martinsos I'm not sure I follow how to call queries without the useQuery hook.

From the docs I had initially understood that wasp would magically add the context, even when calling the function without the hook. Per your last comment, that is not the case, though, and I'm still confused on how to actually do it.

It seems that to directly call the query I must pass the query cache key, plus the user (as context). The advanced usage section for actions says we can get the queryCacheKey from the "operation" itself, but that doesn't work:

import { getTasks } from 'wasp/client/operations';

// ...

const { data: tasks } = await getTasks(getTasks.queryCacheKey, { user });

I would suggest adding to the docs an example of how to call queries with entities without the useQuery hook. I would also say the concepts valid for both queries and actions could be in a separate section that appears before queries and actions, perhaps directly in Overview.

edit: opened a question in discord too.

sodic commented 4 months ago

@leoschet As mentioned on Discord, this issue deals with calling Operations from the server, so it's not really applicable to your situation.

However, the advanced usage docs you linked do talk about calling Operations from client, and you've successfully caught two mistakes. I've created an issue for fixing the docs and improving the API: https://github.com/wasp-lang/wasp/issues/1923

Thanks!

sodic commented 3 months ago

Here's a proposal for the API for calling Operations directly on the server.

I'll explore the API on an authenticated Query. Actions are analogous, and unauthenticated operations offer a subset of what's shown here (I show an example at the end of each section).

1 Proposed API

Defining the Operations remains the same, but I'll give a definition to emphasize the relationship between the types.

Assuming an authenticated Query getFoo defined as:

import { type GetFoo } from 'wasp/server/operations'

export const getFoo: GetFoo<Input, Output> = async (args, context) => {
  // ...
}

We want to enable our users to do this:

import { getFoo } from 'wasp/server/operations'

const payload: Input = { ... }
const user: User = prisma.user.findUnique(...) // This this their 'userEntity'
const result: Output = await getFoo(payload, user)

// If 'getFoo' wasn't authenticated
const result: Output = await getFoo(payload)

1.1 Alternative APIs

Here's a list of things I considered but gave up on. You can try to change my mind if you disagree :)

Users pass in an AuthUser object instead of a User object:

Users pass in userId: string instead of a User object:

Users pass in either a User object or a userId: string

2 Implementation of the proposed API

Assuming we go with the proposed API, here's what we have to do.

The true type of getFoo (as defined above) is:

function getFoo(
  args: Input, 
  context: { 
    entities: Entities,          // All entities the Operation needs, injected by Wasp
    user?: AuthUser | undefined  // AuthUser != User (which they send)
  }
): Promise<Output>

Therefore, to make this work, Wasp would have to generate the following wrapper function and make it available through wasp/server/operations:

// The type 'User' is 'userEntity' defined in the Wasp file.
function getFoo(args: Input, user: User) Promise<Output> { ... }

// If getFoo wasn't authenticated
function getFoo(args: Input) Promise<Output> { ... }

The wrapper has to:

My opinion is that we shouldn't document this.

Let's focus on one official way to do it (i.e., through wasp/server/operations) and make that as easy as possible.

It keeps our API simple and clean.

Of course, our users will still be able to call their operations directly. There's no way to stop them - it's just calling a JavaScript function without anything extra going on (which is also why I don't think we should document it). They might have a hard time constructing the AuthUser object, but I think that's good (it encourages users to use the simpler API).

infomiho commented 3 months ago

0️⃣ I like the proposed user API, I'm just calling operations with the payload and/or with the user 👍

1️⃣ I'd maybe recommend defining the second param as minimal as possible e.g. { id: <type> }, since we only need the ID to construct the full AuthUser.

2️⃣ Keep in mind that this would be a bit expensive since it would fetch:

If they are calling the action in an another action - maybe they already have the constructed AuthUser which they can forward to the called action for free in terms of DB queries. I'm not sure if we should be optimizing this - but I wanted to point out the hidden cost which we might avoid.

Either we accept { id: string } | AuthUser or we have some sort of composable helper method like getFood(payload, createAuthUser({ id }) 🤷

sodic commented 3 months ago

Great points!

I'll address each of them and give my conclusion.

Answers

1️⃣ I'd maybe recommend defining the second param as minimal as possible e.g. { id: }, since we only need the ID to construct the full AuthUser.

Hmm, interesting idea. I was about to accept it, but then realized it suffers from the same problem the raw ID suffers from - it's easy to address a non-existing user.

Also, the type the users see ({ id: string }) is not as pedagogical as User and I'm afraid it might steer them away from what we want them to do - query the database.

Btw, { id: string } | AuthUser resolves to just { id: string } because one is a supertype of the other (just like number | 4 == number). So users would just see { id: string }.

Right after typing that, I figured that we can fix it with overloads (playground). They provide great DX, actually.

2️⃣ Keep in mind that this would be a bit expensive since it would fetch...

That is true. There's only a single way it could be made more efficient and it's, as you said, by asking them to always pass an AuthUser object.

They'd still have to fetch it themselves, but yes, if they already have it, then it's an improvement.

getFood(payload, createAuthUser({ id })

As you might have guessed, I'm not a fan of this kind of API. :smile: I'd rather sometimes do two an extra database Query then expose our plumbing.

Conclusion

For the long term

I like to option of doing User | AuthUser (via overloads) and acting differently depending on which one we get.

If you really like the "pass me only the ID option", I'm also OK with { id: string } | AuthUser (via overloads).

The only question is whether we should add a raw string to the mix, but I think we can skip that until someone asks for it.

For the short term

Should we maybe ignore all possible optimizations and just implement the User or { id: string } solution?

This API is not super popular, and we might be optimizing prematurely.

Martinsos commented 3 months ago

Nice analysis!

  1. [Controversial]: what about calling it like await getFoo(payload, { user })? And then merging that context, which is really the second argument, with whatever context we wanted to inject. It seems more logical to me, because you are calling it the same way it was defined. Seems a bit confusing that you defined it to take { user } but not it takes just user.
  2. I vote for AuthUser as an arg type, potentially User | AuthUser, for all the arguments that were already written. The biggest argument for me is that most often they will be calling it from another Operation, and they already have AuthUser there. If not, and they are calling it from somewhere else like db seed function or setup function, they should be able to easily obtain at least User. This contributes to the consistency of the APIs between how I define the function and how I am calling it.
sodic commented 2 months ago

One more (big) detail I haven't thought about.

Let's put aside the issue of how exactly we pass the user (i.e., User vs AuthUser vs { user: AuthUser } and others).

We agreed it makes sense to expose an operation like this:

const result = await getFoo(payload, user)

But what about operations that don't expect any arguments, for example:

import { type GetBar } from 'wasp/server/operations'

export const getBar: GetBar<void, Output> = async (_args, context) => {
  // ...
}

We could make the number of positional arguments depend on whether the operation expects arguments:

const foo = await getFoo(payload, user) // Expects an argument
const bar = await getBar(user)          // Doesn't expect an argument

But this looks a little janky. Moving the user to the front doesn't solve the problem - the number of arguments now depends on whether the operation is authenticated:

const foo = await getFoo(user, payload) // auth: true
const bar = await getBar(payload)       // auth: false

Any ideas on what we should do? The only thing I can think of is ditching positional arguments in favor of objects (they do have their benefits anyway, see https://github.com/wasp-lang/wasp/pull/554#discussion_r847449529):

// auth: true, takes arguments
const foo = await getFoo({
  payload,
  user,
})

// auth: true, doesn't take arguments
const bar = await getBar({
  user,
})

// auth: false, doesn't take arguments
const baz = await getBaz({}) // kind of dumb
const baz = await getBaz()           // kind of inconsistent

@infomiho and @Martinsos please weigh in on those, preferably with something creative that's a gamechanger :)

Martinsos commented 2 months ago

@sodic to me it seems pretty clear, but maybe I am missing something: I would stick with how the function is defined.

That means the call would be:

const result = await getFoo(payload, user)

And if there are no arguments, then it is just

const result = await getFoo({}, user)

which makes sense, because arguments are provided as a single object (payload) where arguments are fields, and in this case there are none so that objects is empty, and all is good.

Martinsos commented 2 months ago

That said, I would even go with

const result = await getFoo({}, { user })

without feeling bad about it. I don't know if we will want to be putting something more in context in the future or not, and this leaves it flexible while also keeping it consistent with how it is defined.

Look at it this way:

  1. Positional arguments are not great when there are optional arguments, so let's say we like non-positional arguments (named arguments). If so, we are going with object.
  2. We can't have one single object, because we have args which can be named anything, and we have "options" or we call it "context" which has known fields. So we really have two groups: args, options(context). So it makes sense to have await getFoo({arg1, arg2, ...}, { user, entities, ... })

We could in theory go with await getFoo({args: {arg1, arg2}, context: {user}}), but we are quite confident that split into args and context is enough, so this is probably too much.

So again, we end up with getFoo({...}, {...}) as a way to go. It looks the same as the signature, it gives flexibility to us to allow putting more stuff in context if needed.

sodic commented 2 months ago

@Martinsos thanks for the input. Let's leave all the user stuff aside for now and look at an unauthenticated query.

which makes sense, because arguments are provided as a single object (payload) where arguments are fields, and in this case there are none so that objects is empty, and all is good.

You'd be 100% percent right if the bold part were true, but unfortunately, it's not :sweat_smile:.

We can (and often do) have:

export const getFoo: GetFoo<number, Foo> = async (n, context) => {
  const x = args + 12 // n is just a number
  // ...
}

In other words, args can be any JS type (more precisely, SuperJson type). It's not limited to an object with fields.

In practice, the {} idea would look like this:

getFoo({ id }) // getFoo: GetFoo<{ id: number }, Foo>
getFoo(13)     // getFoo: GetFoo<number, Foo>
getFoo("bar")  // getFoo: GetFoo<string, Foo>
getFoo([1, 2]) // getFoo: GetFoo<number[], Foo>
getFoo({})     // getFoo: GetFoo<void, Foo>
getFoo({})     // getFoo: GetFoo<{ id?: number }, Foo>

This doesn't seem right. More importantly, since void is not the same as {}, it opens the API up to many potential type errors.

I'll give two examples.

Example 1

Consider this operation:

export const getFoo = async (args: { filter?: string } = { filter: "active" }) => {
  // ...
}

The author wants to:

How would you call it and trigger the default filter (active only)?

getFoo({}) // means args is {}, i.e., no filter

Yes, this is not peak API design, but that's up to our users :)

Example 2

Let's assume we've defined getFoo as GetFoo<void, Foo>. Now consider the following call site:

getFoo({})

Changing getFoo's definition to GetFoo<{ id?: string }, Foo> won't report any type errors for this call. Users would probably like to know they should update their call site and send an empty object explicitly (which means "I don't want to specify an id").

Martinsos commented 2 months ago

Ah whoops, ok I forgot it can be anything, you are right!

Ok, but in that case, we just stick with whatever they defined, right?

If they don't use the first argument, and define it as void, what would you expect them to pass at call site? Since my TS game is not yet 100%, I asked ChatGPT, and it said we should be using void for a parameter that is not used (is it making sense?). Suggested instead defining it as getFoo(_: undefined, ...), and then passing undefined to it. Which sounds ok to me?

If not, what do you think is the best alternative? Allowing dropping first argument? But then it becomes quite confusing, if tehre is just one argument, which one did you drop, the first one, the second one, ... , right?

sodic commented 2 months ago

Had a call with @Martinsos. We are going with: