Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor/service method #381

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Refactor/service method #381

wants to merge 15 commits into from

Conversation

Paulijuz
Copy link
Member

@Paulijuz Paulijuz commented Jan 12, 2025

This PR refactors the implementation of service methods. This makes life easier and also simplifies the implementation of tests.

Major changes include:

  • Merged ServiceMethod and ServiceMethodHandler into one as they were almost identical.
  • Merged Action and ActionNoData
  • Added validation for params. I think this is necessary as the data comes from the client and can in theory be anything. While unlikely (from non malicious actors at least), it doesn't hurt to be safe.*
  • Added functions bindParams(action, ...) and bindData(action, ...) as a replacment for action.bind(null, ...). They are functions which take in an action as their first argument and bind the relevant argument.**
  • No need to explicitly specify withData when creating service methods. This is inferred from whether or not a data validation is present.
  • In the same way as above, whether params are required is also inferred.
  • To specify no auther the string 'NO_AUTH' can be passed.
  • Since I'm lazy .client('NEW') has been changed to .newClient(). The latter takes fives keystrokes (. + n + tab + ( + )) as opposed to eight (. + n + tab + ( + ' + N + tab + )).
  • I have translated all the services witch used ServiceMethod the new system. I have also translated a few more service methods to test out the new syntax while developing it.
  • I have removed the index file in each service folder FOR NOW.***

*Note on params validation: I have currently implemented param validation with bare zod schemas. I did not use Validations as two steps of validation is unnecessary for params I think (and also maybe for data?). I did not change the Validation as that would be out of the scope of this PR. We need to discuss how it should be implemented properly as having two ways of creating validation is messy.

**Note on binding: I did not manage to implement the binding functions with dot notation as server actions cannot be objects, nor can they be functions with custom properties, thus this was the only way to implement something close to what we wanted. Another thing: Since server actions will be "normal functions" I also think that we should only bind arguments wherever necessary, as opposed to binding them first and the calling them immediately as was done previously. I.e. we should avoid writing bindParams(action, ...)() (the new version of action.bind(null, ...)(), and simply do action(...) instead.

***Note on index files for service: I think ideally all services for a model should be defined in the same file (index.ts or maybe service.ts, we need to decide on a name. I think Johan doesn't like index files if I'm not mistaken), in stead of it importing all the functions from create.ts, read.ts, update.ts and destroy.ts. I have not had the time to do this so I have simply removed these files for now.

Minor changes include:

  • Changed the names of the action function, auther instances, and auther.ts files to lower case as to adhere to styling convention.
  • Created a function for creating pagination params schemas to simplify creating pagination params. This can be changed at a later date when we figure out how params should be properly validated.

Things that I think still need to be done:

  • Merge all create.ts, read.ts, update.ts, destroy.ts in services into one file. Where alle the definitions of the service methods live in a single object. Preferably with a naming convention like userService, lockerService, cmsImageService, etc...
  • Merge all create.ts, read.ts, update.ts, destroy.ts in actions into one file. Most files with the new system will be only a few lines, so fragmenting the definitions will be more disorganized than organized I think.
  • Write the final implementation for params schema and/or refactor/remove Validation.
  • Rewrite all the "old" services to use ServiceMethod.

@Paulijuz Paulijuz mentioned this pull request Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant