stemmlerjs / white-label Goto Github PK

https://shhgit.darkport.co.uk/
https://github.com/stemmlerjs/white-label/blob/master/src/modules/notification/services/slack/index.ts

Hi,

While using this repository as an example to build my app, I saw an opportunity to contribute cleaning the code.
Default parameters are often cleaner than shorting circuiting (or thernary operator, in this case).

Be aware that if you use default parameters, your function will only provide default values for undefined arguments. Other "falsy" values such as '', "", false, null, 0, and NaN, will not be replaced by a default value.

So, if this is a problem in this case, please, let me know.

Bad:

Good:

public clientError (message = 'Unauthorized') {
    return BaseController.jsonResponse(this.res, 400, message);
}

Reference:
https://github.com/ryanmcdermott/clean-code-javascript#use-default-parameters-instead-of-short-circuiting-or-conditionals

For example, only users with read users can retrieve users info. Related code may exist in many usecases in user module. How to do that?

Hi!
Sequelize 5 supports TypeScript, so you can write all Sequelize models in TypeScript and get rid of the vanilla JavaScript code.
Btw, why are the Sequelize models not in the src folder?

In this commit (lines 27, 28) for your UserMap.ts mapper, you are attempting to access the Result value from the ValueObjects when they might not necessarily have been created properly:

 public static toDomain (raw: any): User {
    const userEmailOrError = UserEmail.create(raw.user_email);
    const userPasswordOrError = UserPassword.create(raw.user_password);

    const userOrError = User.create({
      email: userEmailOrError.getValue(),
      password: userPasswordOrError.getValue(),
      firstName: raw.first_name,
      lastName: raw.last_name,
      isEmailVerified: raw.is_email_verified,
      username: raw.username
    }, new UniqueEntityID(raw.base_user_id))

    userOrError.isFailure ? console.log(userOrError.error) : '';

    return userOrError.isSuccess ? userOrError.getValue() : null;
  }

If either UserEmail.create(): Result<UserEmail> or UserPassword.create(): Result<UserPassword> fail, then you'll see the following error when you call getValue on the Result instance:

"Can't get the value of an error result. Use 'errorValue' instead."

I'm interested in your architecture, so I was wondering if this is something you intend to rectify, and if so, how you'll do it.

Might you combine the results, perhaps:

 public static toDomain (raw: any): User {
    const userEmailOrError = UserEmail.create(raw.user_email);
    const userPasswordOrError = UserPassword.create(raw.user_password); 
    
    if (!Result.combine([userEmailOrError, userPasswordOrError])) return null;

    const userOrError = User.create({
      email: userEmailOrError.getValue(),
      password: userPasswordOrError.getValue(),
      firstName: raw.first_name,
      lastName: raw.last_name,
      isEmailVerified: raw.is_email_verified,
      username: raw.username
    }, new UniqueEntityID(raw.base_user_id))

    userOrError.isFailure ? console.log(userOrError.error) : '';

    return userOrError.isSuccess ? userOrError.getValue() : null;
  }

Or, are you asserting that both value objects will never be erroneous because they are coming straight from the database where the integrity and validation of data are guaranteed?

Thank you.

Hi Khalil Stemmler,
Your articles about DDD are so useful. Currently, I'm practicing some skills from DDD such as strategic design, event storming, etc. Therefore, I realy want to contribute to this project by designing and implementing one service/module with Golang.

Hi @stemmlerjs ! Thank you for your efforts and your lessons. I've been reading a LOT of your blog and bought your book to learn more about DDD. I'm wondering why you've underscored _domainEvents here? From what I've learned from you, we don't have to do this in typescript since we already have access to the private keyword. Is there something I'm missing?

why there is no activity for this project? Also i don't the front end part.

Hi,
I am trying to build this code, it is giving lot of error message,

Please let me know is this latest code base,

Thanks
Mohsin

Hello.
In ./src/core/domain/AggregateRoot.ts there's no constructor present.
However, in ./src/modules/users/domain/user.ts, class User (which extends AggregateRoot), has a super(props, id) call in its constructor.
Where's AggregateRoot's constructor?
Thanks.

Hi have seen there is a Facebook and Google OAuth Providers but is there a way to have a Custom Jwt Token as auth Bearer Token for API authentication. And OTP based authentication too just a suggestion.

Hi,
I just found this. Niiice!
Looks like a good idea for a very clean, general purpose architecture. I took it for a drive test and I run into:
headers already send situation

The issue lies here:

public execute (req: express.Request, res: express.Response): void {
    this.req = req;
    this.res = res;
    this.executeImpl();
 }

If I convert

userRouter.post('/',
  (req, res) => createUserController.execute(req, res)
)

to

userRouter.post('/', async (req, res) => { 
     const createUserController = new CreateUserController(/useCaseGoesHere/)
     await createUserController.execute(req, res)
 })

it work.

export class User extends AggregateRoot<UserProps> {

    get id (): UniqueEntityID {
        return this._id;
    }

    get userId (): UserId {
        return UserId.caller(this.id);
    }

    get email (): UserEmail {
        return this.props.email;
    }
        .
        .
        .
}

Why are there two id attributes in class User? one is named id and the other userId.
Why not use id : UniqueEntityID for all aggregates?
Also, the Function.caller seems to be deprecated.

Is there is a reason why it's not returning Result ?

It seems like you create a singleton for each controller and use a controller's execute method as a handler for express routes. BaseController.execute is synchronous, but it calls the sub-class's executeImpl method, which might be asynchronous. executeImpl might call BaseController.conflict, BaseController.fail, etc. which rely on BaseController.req and BaseController.res.

If multiple requests to the same endpoint come in faster than they can be handled, the multiple calls to execute will overwrite the req and res fields. Then when the executeImpl method finally calls this.conflict, won't it be using the wrong res for res.send?

Instead, should we not create a new controller each time we want to handle a route?

E.g.,

- import { createUserController } from '../../../useCases/createUser';
+ import { CreateUserController } from '../../../useCases/createUser/CreateUserController';
+ import { createUserUseCase } from '../../../useCases/createUser'; // export the use case singleton in this file

const userRouter = express.Router();

userRouter.post('/',
-   (req, res) => createUserController.execute(req, res)
+   (req, res) => {
+     const controller = new CreateUserController(createUserUseCase);
+     controller.execute(req, res);
+   }
)

can someone please explain why is it not strict thanks