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

switching to express router #11

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

Conversation

MoustafaJazzar
Copy link

No description provided.

Copy link
Owner

@yebrahim yebrahim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for sending this, the code is much cleaner now. let's coordinate with @mohamedsaleh1984 since he'd sent some changes in the same area, or at least wait for him a day and then merge if there are no updates there. :)

import { app } from './app';
import { initDb } from './datastore';

const initServer = async () => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice cleanup here, thanks.

@@ -15,3 +15,10 @@ CREATE TABLE posts (
postedAt INTEGER NOT NULL,
FOREIGN KEY (userId) REFERENCES users (id)
);

CREATE TABLE likes (
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a new migration file with these changes, something like 002-add-likes.
the point of database migrations is that they're incremental, each change is isolated in a file so that the migration library can decide which changes don't exist in the db yet and need to be run.

@@ -1,24 +1,26 @@
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: curious what happened with the formatting here. I was using two-spaces indentation, can we keep it this way? :)

// Like APIs
export type CreateLikeRequest = Like;
export type CreateLikeResponse = {
message: string;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the backend to send these back. the client should expect a successful (2xx) request to have acted on the like, and returning the same object the client sent in the request isn't probably all that useful. I'd keep this an empty interface until we have something useful to add here.


// MIDDLEWARE
app.use(express.json());
app.use(logger('dev'));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know much about morgan, but it seems like a logging middleware, so we can't use it for manual logging correct? if so I think we'll need to use something different, or even cook up our own thing here. I'd like to not have to do console.log/error/warn and instead use the same logger with the same format for consistency.

@@ -0,0 +1,2 @@
export * from './userRouts';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in both file names: userRoutes and postRoutes.

@yebrahim
Copy link
Owner

yebrahim commented Mar 7, 2022

@MoustafaJazzar I'd like to move forward with the next parts, namely adding proper authentication. Would you be able to handle the comments and merge this PR soon? Otherwise you could rebase or merge my changes. :)

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.

2 participants