Automatic CRUD

[infomiho]

Fixes #1171

What has been implemented

crud tasks {
  entity: Task,
  operations: {
    get: {},
    getAll: {
      isPublic: true
    },
    create: {
      overrideFn: import { createTask } from '@server/tasks.js'
    },
    update: {},
    delete: {}, 
  },
}

• Wasp language now has the crud declaration
  • It accepts:
    • entity for which the CRUD is built
    • operations dict which has keys for each operations
    • under each key the user can declare isPublic and a custom implementation with overrideFn
• checks for
  • entity must have an @id field
  • at least one operations must be defined (otherwise, what's the point)
• server routes
• client API via importing import { tasks } from "@wasp/crud/tasks.ts" and using it like:
  • tasks.getAll.useQuery()
  • or tasks.create.useAction()

What's left to figure out

• a lot of our users connect the entity with the current user e.g. Task belongs to User, can we make this work out of the box?
• types for the client API
• do we need to do something for full-stack type safety (import the types for the client from the backend?)
• do we want to validate the input received from the client? (other than delegating to Prisma for required fields check)
• do we want to include all relations (user User @relation(...) -> we include user instead of just userId)

[shayneczyzewski]

Overall, this is looking really good @infomiho! I think once you add the override notion we discussed (hopefully it can work 🤞🏻) and the type info is solved, we will be there! 🥳 Great job so far and fast work! Also, the extra app made it super fast to play with, thnx for that :D

[sodic]

Great work on getting this to the PR stage in just a week! Also, sorry for the late review.

Thanks for building the simple example app. It made comprehending and testing out the feature very painless. I tried it out, and everything worked properly.

I started the review by looking at the TypeScript as you requested, but most of my reservations ended up being about the API and DX. In fact, the API and DX are the only things I commented on, leaving out all implementation comments (as most of them will likely become irrelevant as we iterate on the feature).

When we settle on the feature's final form, I'll take a more detailed look at its implementation.

[sodic]

We've haven't yet taken this approach with our API (i.e., a hook you pass your stuff into). We always takes the type-assignment approach (i.e., you assign the expected type on the left-hand side):

export const overrides: CrudOverrides = {
  // ...
}

I don't necessarily think this is better, but we should probably keep it consistent at this point and revisit in the future.

[Martinsos]

This is a good point, but if hook works much better, maybe we can go with it, as an experiment? I would want us to do stuff in worse way just because we did it like that before :D. And this is not an argument to remove Haskell and DSL also :D.

[Martinsos]

@infomiho what are your thoughts on this?

[infomiho]

That's an interesting one, since we changed the overrides approach to be more like what have for operations, the concept of createCrudOverrides doesn't exist anymore.

I could implement createGetOverride, createGetAllOverride etc. functions and expose it for each crud? I'm on the fence but it might be easier for users to use that mechanism?

[Martinsos]

But you now implemented these via DSL, didn't you?

[infomiho]

But you now implemented these via DSL, didn't you?

The discussion at hand doesn't have to do anything with DSL but rather the way we provide Typescript types when defining the override implementations.

First approach is to export types like GetQuery and then users import them and use like this:

import { GetQuery } from '@wasp/crud/tasks'

const getOverride: GetQuery<Input, Output> = (args, context) => {
  // args and context are typed here
}

Second approach is to offer an id function that provides types like this:

import { createGetOverride } from '@wasp/crud/tasks'

export const getOverride = createGetOverride<Input, Output>((args, context) => {
  // args and context are typed here
}) 

So, it's a matter of trying out a new approach or doing what's familiar 🤷 Right now, we are doing what's familiar.

[Martinsos]

Got it! Ok, will let you judge what you think it the best! It does make sense to keep things consistent though.

[sodic]

Great work on getting this to the PR stage in just a week! Also, sorry for the late review.

Thanks for building the simple example app. It made comprehending and testing out the feature very painless. I tried it out, and everything worked properly.

I started the review by looking at the TypeScript as you requested, but most of my reservations ended up being about the API and DX. In fact, the API and DX are the only things I commented on, leaving out all implementation comments (as most of them will likely become irrelevant as we iterate on the feature).

When we settle on the feature's final form, I'll take a more detailed look at its implementation.

[Martinsos]

Took a quick look, I quite like it!!!

I think this is the right direction, as we discussed. Even if this first version will end up a bit lacking in some areas, or a bit conflicting with some parts of Wasp, I think it will open a new area for us and allow us to explore and experiment more, and that is very important and a great next step. Others and also us will be able to play with it, reason about it, complain about it, and that will enable us to learn and more forward in this direction.

That said, I do like the current solution already quite a bit.

I wrote above about override ideas: I think we should do full override, but also hooks. I explained more above in another comment.

As for Wasp DSL, I have a different suggestion for syntax, which is a bit of a spin on the current one:

crud Tasks {
  entity: Task,
  operations: {
    create: {},
    getAll: { public: true }
  }
}

So main difference is that use has to explicitly enumerate the operations they want to get.
Why I think this is an improvement:

1. It is easy to throw a look here and see what is supported - Wasp style, less conventions/magic, more explicit/readable stuff.
2. We can change stuff in the future without breaking much -> e.g. we can add a new method, and it will not magically get added to their code without them aggreing on it.
3. They can't accidentally expose an operation, for example Delete because they forgot it is there.
4. It gives us a good place in DSL to provide additional configuration for each method/operation. I am not sure where we would do this otherwise.

Btw, there is a couple ways we can play where to make it a bit shorter:

crud tasks (Task, {
  operations: {
    create: {},
    getAll: { public: true }
  }
})

or even

crud tasks (Task, {
  create: {},
  getAll: { public: true }
})

This is all supported by current Wasp syntax.

As for the overrides, this is what overriding the whole method to use existing operation could look like with this syntax (and also attaching custom operations/methods):

crud tasks (Task, {
  operations: {
    create: { use: createTask }, // override
    getAll: { public: true },
    custom: [
      cloneTask,
      toggleTask
    ]
  },
})

action createTask ...

action cloneTask ...

action toggleTask ...

[Martinsos]

Ok, I took a look at the RFC where Wasp DSL syntax was discovered, very nice job there, you covered all of this!

I still think user should explicitly define which methods they want to use, due to arguments above. It is Wasp style + they can't expose method by accident. If I am correct, both @matijaSos and @sodic share this opinion with me.

Another thing is if operations should be defined as

  operations: {
    getAll: {},
    create: { public: true }
  }

or

  operations: [GetAll, Create],
  config: [
    (Create, { public: true })
  ]

I kind of prefer the first approach, but I could also be ok with the second one if others prefer it!

[Martinsos]

@infomiho this is looking pretty good, seems to me like we are close to being done!

I left some comments, check them out.

Some things to think about:

1. Would be great if you can go through older comments that are still open and resolve them or answer them, so we clear those up.
2. You asked a good question of validating the data that is being sent, for create and update methods. Well, I guess even for get and getAll hm. Seems to me like that would be pretty important for real usage to make sense. Even if we don't do it now, in this PR, I would love if that by the end of this PR we have a clear picture of what we would need to do there / what are our options -> so have a plan for it.
3. You mentioned relations, especially the one to User -> for this also, same as for validation, it would be great if we at least make a plan in this PR. Look at some use cases, look at some options, propose some solutions. Even if we don't it right now, we would leave with some kind of a plan for the future.

Without (2) and (3), I think this feature will likely have to remain experimental, more of a showcase. But with (2) and (3) it feels like it could be a full feature.

[Martinsos]

@infomiho this is looking pretty good, seems to me like we are close to being done!

I left some comments, check them out.

Some things to think about:

1. Would be great if you can go through older comments that are still open and resolve them or answer them, so we clear those up.
2. You asked a good question of validating the data that is being sent, for create and update methods. Well, I guess even for get and getAll hm. Seems to me like that would be pretty important for real usage to make sense. Even if we don't do it now, in this PR, I would love if that by the end of this PR we have a clear picture of what we would need to do there / what are our options -> so have a plan for it.
3. You mentioned relations, especially the one to User -> for this also, same as for validation, it would be great if we at least make a plan in this PR. Look at some use cases, look at some options, propose some solutions. Even if we don't it right now, we would leave with some kind of a plan for the future.

Without (2) and (3), I think this feature will likely have to remain experimental, more of a showcase. But with (2) and (3) it feels like it could be a full feature.

Regarding (2) and (3) -> before we finalize this PR, could you, as a part of it, describe a bit a short plan on how would you handle these two points, your current ideas? I am ok with not handling them in this PR as that would be too much, but I would love if behind us we leave GH issues with some initial thoughts on both of these points, that later we can continue from. We could do that later, sure, but now it is fresh!

[Martinsos]

@infomiho looking good to me! I think we mostly solved everything.

I will approve the PR as I think all the major stuff is in order, although there are two things I think are still needed to call it done:

1. Gh issue(s) for next steps in CRUD: validation, auth, ... . I think it is important to create those now while we are still fresh. We can also include them in docs (link to them).
2. Docs

I am not sure if some of this already got done or not, if yes great.

[infomiho]

I've added the docs in my last couple of commits 👍

I'll be creating the issues before I merge this PR

[Martinsos]

Reviewed docs!

[Martinsos]

Great @infomiho, and nice job on writing all that documentation! I think it is looking great - I left last two comments, although they are connected. I am approving anyway, so once you are happy with those, feel free to merge.

[Martinsos]

I would consider structuring start like this:
0. caution Early preview, as you have now.

1. Shortly what CRUD is.
2. Info about how automatic implementations work -> I maybe wouldn't even go into details of showing Prisma code as you have, I would say it by words -> save all the data, update all the data given, ... . Or if we are going with Prisma code, try to condense it a bit, so it is easier to quickly scan with eyes.
3. Warning/info about current limitation of automatic/default implementations and then, as part of that, mention and link to future plans for validation and auth.

Why like this? Because they quickly get to what feature is about, and get interested, otherwise if start with limitations, or future, it is kind of out of context.
So first explain, excite, then warn, and show future.

[infomiho]

I've tried to make it as readable as possible while giving enough details to the user :)

[infomiho]

I've created an issue for further improvements that can benefit the CRUD operations. #1253