add plop based boilerplate generator#67
add plop based boilerplate generator#67Sofyrus wants to merge 9 commits intojoshuaalpuerto:masterfrom
Conversation
|
This pull request introduces 1 alert when merging d9e4179 into 79d2bca - view on LGTM.com new alerts:
|
|
Wow!! I will look into these as soon as I could! Thanks @Sofyrus |
joshuaalpuerto
left a comment
joshuaalpuerto
left a comment
There was a problem hiding this comment.
First of all I appreciate for making this contribution: 🎉
I found some couple of issues when running repository, routes and domain.
Right now as I understand the developers need to do yarn generate or npm generate 4 times. It would be great if it's just 1 selection and base on their answers it will generate the necessary files domain, routes and repository.
So far it's looking good, I have some small comments and suggestion, I will contribute as-well when I can.
| // Generate index.js and index.test.js | ||
|
|
||
| let appRelativePath = "src/app/{{camelCase name}}/"; | ||
| let routeRelativePath = "src/interfaces/http/modules/{{camelCase name}}/"; |
There was a problem hiding this comment.
Filename convention -
camelCaseshould never be used. This leavessnake_caseandkebab-case, I prefersnake_casefor file.
No worries if you cannot edit it man I will do it within this week 👀
There was a problem hiding this comment.
Question do we need to generate the domain object as well ? that would imply asking for some initial properties like sequelize does for it's models?
There was a problem hiding this comment.
Good question, we can just have the domain object without properties for now (probably add some dummy property).
Anyway, our goal is to remove some boilerplate from their end so I think this is enough.
| const getUseCase = get({ {{#if InjectRepository}} {{camelCase repositoryName}},{{/if}} logger}) | ||
| const postUseCase = post({ {{#if InjectRepository}} {{camelCase repositoryName}},{{/if}} logger}) | ||
| const putUseCase = put({ {{#if InjectRepository}} {{camelCase repositoryName}},{{/if}} logger }) | ||
| const deleteUseCase = remove({ {{#if InjectRepository}} {{camelCase repositoryName}},{{/if}} logger }) |
There was a problem hiding this comment.
I think we don't need to include logger here by default.
There was a problem hiding this comment.
I have found myself injecting logger in app layer in multiple scenarios. Can you expand/elaborate the reasoning for not having logger here
There was a problem hiding this comment.
Well I don't have hard reason. We can import it just add logger from container.cradle as well.
| return router | ||
| } | ||
| return router; | ||
| }; |
There was a problem hiding this comment.
I think you have used prettier here instead of standard?
There was a problem hiding this comment.
yes, it's the default throughout our VS Code setup
There was a problem hiding this comment.
I was actually planning to move this also to prettier because most of developers are working with that in their frontend code.
Created an issue here. Let have that in separate PR.
|
This pull request introduces 14 alerts when merging 69a8b2c into 79d2bca - view on LGTM.com new alerts:
|
|
This pull request introduces 14 alerts when merging 90b7892 into 79d2bca - view on LGTM.com new alerts:
|
|
This pull request introduces 14 alerts when merging d39fc6f into 79d2bca - view on LGTM.com new alerts:
|
joshuaalpuerto
left a comment
joshuaalpuerto
left a comment
There was a problem hiding this comment.
Awesome job man. I added some additional small comments.
Also, I see you have some changes related to this issue. Let's put that in different PR since that is a different concern.
| type: "input", | ||
| name: "repositoryName", | ||
| message: "Name of existing repository.", | ||
| description: "Only needed if previous answer was in affirmitive?", |
There was a problem hiding this comment.
We can use this plugin, so we don't have to show the next question if it depends on the previous one.
| "bcrypt": "3.0.8", | ||
| "body-parser": "1.18.3", | ||
| "bull": "^3.18.1", | ||
| "bull-board": "^0.10.0", |
There was a problem hiding this comment.
Thank you so much for contributing man, but let's put this to separate PR.
| const date = require('./infra/support/date') | ||
| const repository = require('./infra/repositories') | ||
| const eventPublisher = require('./infra/eventPublisher/publisher') | ||
| const scheduler = require('./infra/bull') |
There was a problem hiding this comment.
This is a different concern so let's put it to a different PR.
4 participants
This PR will add to boilerplate the capability to quickly generate the files for app and routes and will also update router.js inside modules/http to have an entry for the newly created route.
This is the first draft and the generator can be improved with time
Note: Since we are actively working using this and one of the team members added event based pub sub skeleton that is now also the part of this PR @joshuaalpuerto