Feedback

[ricayanzon]

Hi,

while I really appreciate and like how you implemented the concepts of clean architecture, there are some bugs/unclean things in your code. To name a few:

• I think all methogs inside login.usecases.ts should be split up into several different files, group them in the same folder, but a use case should only contain one method afaik (execute or handle)
• I personally prefer kebab-case for file names, e.g. is-authenticated.usecase.ts instead of isAuthenticated.usecases.ts and it should actually be singular, as it is a usecase inside the usecases folder
• in your tests, you sometimes have async functions and pass the done callback. you shouldn't do that (either make the test function async or pass a DoneCallback. don't do both). there's an issue actually requesting that both should be usable, but since the issue is still open, this is not possible yet [jest-circus] Support both done callback and async test simultaneously jestjs/jest#10529
• in the jwt.strategy.ts file (and I think some places else), you use process.env.JWT_SECRET to get the JWT secret, whereas it should be accessed using the dedicated method inside environment-config.service.ts (why else did you implement them?)
• exceptions.service.ts some exceptions start with capital letters and some with lowercase - just a small detail but it'd be nicer if they all start the same
• are you're e2e tests really working for you? it seems to me that you're not mocking the database, and as login requires to check the DB for existing users, it fails when I try to run the tests
• some tests don't really seem to test anything or not what the description implies. e.g. the last test in auth.spec.ts says it 'should return an array to invalid the cookie' but you're not really invalidating anything as you're only calling the isAuthenticated.execute method, which executes the getUserByUsername from the user repository. so it actually only returns a UserWithoutPassword or null (this is not invalidating a cookie afaik) and also, in your test, you're mocking the result from getUserByUsername in (adminUserRepo.getUserByUsername as jest.Mock).mockReturnValue(Promise.resolve(user)); - so you're basically only comparing the returned value from the usecase and the mocked user. and since the mocked version of getUserByUsername returns the mocked user, you're not really testing anything.

I want to emphasize once more that I really appreciate you're effort to implement this and to make it available to anyone. I merely wanna help improve the quality with my comments. Hope it helps.
Thanks!