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
Feedback #22
Comments
Thanks much for your Impressive project @OultimoCoder
I am trying Clouflare Workers with this great project. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hey OultimoCoder, I've been working with your boilerplate for a while now and have some initial feedback on it. First of all though, thanks for your effort in putting it together - it's a definite time saver.
At the moment your API contracts are enforced in the validations folder by code such as user.validation.ts. E.g. you have:
The naming convention I think could do with some improvement. E.g. I have renamed
createUser
tocreateUserValidator
andCreateUser
toCreateUserRequest
. I have also renamed thevalidations
folder tocontracts
and changed the filenames, e.g.auth.contract.ts
. Having these is great, but I feel they should also be used in your integration tests. Your tests use this:let newUser: MockUser
which in turn is based onexport type MockUser = Insertable<UserTable>
. I can see why you do it, because you need to insert the test data and create the post request. However if you refactor your user fixture along these lines, you can stay true to the contracts but also use them for data population. (Extra user fields in my version!)Your user fixture also implements the UserResponse interface which I think should be made official in the contract file. I implement mine like this:
Ideally there should probably be a corresponding response type for each of the possible requests. This makes your API schema contract pretty solid and you can then:
return c.jsonT<UserResponse>(user, httpStatus.OK as StatusCode)
(although I haven't tried this yet)Not sure this is a great explanation, so happy to upload you better examples if you want.
One less thing to think about is using something like
nanoid
to generate random IDs for objects, in particular user IDs - it just eliminates an attack surface by not having predictable user IDs.I changed
is_email_verified
toverified_at
just to give me more info as to when it was verified.I personally don't like leaking snake case
verified_at
in JSON responses. You can use middleware to convert snake case inbound to camelcase and the reverse on the way out. This is how it looks:If the user wants to change their email address, they should only be allowed to do this following verification. Typically you would create a separate route for this and create a persisted token linked to the event. When the user confirms via the email message, only then is the email changed. I haven't got too far into it, but it looks as if once verified, the user can change his own email address without automatic reverification.
That's it. As I said, thanks for creating this in the first place. Saved me a lot of time.
The text was updated successfully, but these errors were encountered: