-
Notifications
You must be signed in to change notification settings - Fork 38
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
WIP: refactor(warthog): monorepo #287
Conversation
Split Warthog core and cli into packages. closes goldcaddy77#147
> { | ||
async userRoles( | ||
@Args() { where, orderBy, limit, offset }: UserRoleWhereArgs | ||
): Promise<UserRole[]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why these changes are happening? Looking to minimize this diff if possible. Can you try to remove whitespace-only changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took me a while to figure this one out, but looks like it is because of Prettier's version. The master branch resolves to 1.18.2, while my branch resolved to 1.19.1 after my first dependencies review to declare them in the right sub-packages. Using version 1.18.2 "fixed" it. Should we change the dependency to "match exactly" (
instead of ^
) Prettier's version 1.18.2 to prevent it from happening again in the future?
@@ -4,6 +4,7 @@ logs | |||
npm-debug.log* | |||
yarn-debug.log* | |||
yarn-error.log* | |||
*.tsbuildinfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript 3.4 introduced the --incremental
option to enable faster subsequent builds. This flag tells TS to save information about the project graph in a file called .tsbuildinfo
. The next time TypeScript is invoked with --incremental
, it will use that information to detect the least costly way to type-check and emit changes to your project. The incremental option is also enabled by default when you have composite projects. Since we are going monorepo, both of these features can be useful during development and so I added it here.
"ts-jest": "^24.0.2", | ||
"ts-node": "^7.0.1" | ||
}, | ||
"lint-staged": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did these have to be moved out of package.json into rc files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I've just removed the configs from package.json
and created .rc
files, but these files should be .config.js
ideally. In monorepo setups, each package usually contains their required configuration for tooling. However, it is nice to have a common base configuration and, if needed, the package can extend/override the config. We can, for example, have a base Jest config file with ts-jest
and coverage properties predefined and do something like this:
// packages/core/jest.config.js
const baseConfig = require("../jest.base.config.js");
module.exports = {
...baseConfig,
< custom properties >
}
I think it makes sense for prettier
and jest
. lint-staged
would be the only tool configured in package.json
, so I've also created a separate configuration file for it. I intend to revisit this when I figure out how to setup Jest to have an aggregate coverage report.
What do you think about it? Do you prefer these configurations to be in package.json
?
"typeorm-typedi-extensions": "^0.2.3", | ||
"typescript": "^3.7.2" | ||
}, | ||
"devDependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried this built? I image a bunch of these are real dependencies, not devDependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have tried the built modules - in both existing and new projects - and it is working fine. However, that is probably because @warthog/core
have the CLI covered. I did a review of dependencies that each module/sub-packages uses at 931326b, mostly based on import
and require
statements. Do you still see dependencies that should be real dependencies instead of devDependencies (or vice-versa)? Since you know the project better, it would be interesting if you could also review it.
packages/server-express/package.json
Outdated
}, | ||
"publishConfig": { | ||
"access": "public", | ||
"registry": "http://registry.way-dev.com/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was experimenting with Lerna to see how publishing would look like and configured a private registry, but forgot to use --no-git-reset
option (oops). Please ignore this, I've already fixed it.
Hey, thanks for putting so much time into this PR! I've been thinking about moving to a monorepo and your approach seems sane. I'd ask a few things:
I've added some other preliminary comments, but there is a lot of noise more with such a huge diff. Once you clean the diff up I’m happy to do a deeper dive. Thanks again! |
Also, if your use case is open source I’d love to check it out. |
17ce3ae
to
308e9c9
Compare
308e9c9
to
931326b
Compare
Hey! I'm happy to hear that you are thinking about moving to a monorepo. This would have made my work a little bit easier 😄. My intention with this draft PR was to know if you would like to move to a monorepo structure - didn't want to spend more time on it otherwise.
I've kept Most of the diff now relates to file path changes to support the monorepo structure. I'm afraid there isn't a way around it. I also had to recreate the generated code of the examples and looks like some them were not up to date and/or broken, which also increased the diff.
Yes, explanation here. TL;DR it makes sense to have a base config and be able to extend from it when you are inside a monorepo. I must add, however, that this is optional. I can move most of the configs back to
Sure! How do you prefer to do it? I can create discussions in the files/diff, explain key points here in a simple comment or get in touch with you via something like Gitter.
Thank you for this awesome piece of software!
Sadly it is not, but I am going to reproduce a minimum example ASAP and send to you. |
packages/core/src/core/Context.ts
Outdated
@@ -1,4 +1,3 @@ | |||
import { Request } from 'express'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should move this to server express so that we can keep the typing
{ | ||
"printWidth": 100, | ||
"singleQuote": true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep in package.json for now.
@@ -238,7 +238,7 @@ The javascript `Date` as string. Type represents date and time as the ISO Date s | |||
export type DateTime = Date | string | |||
|
|||
/* | |||
The `Float` scalar type represents signed double-precision fractional values as specified by [IEEE 754](https://en.wikipedia.org/wiki/IEEE_floating_point). | |||
The `Float` scalar type represents signed double-precision fractional values as specified by [IEEE 754](https://en.wikipedia.org/wiki/IEEE_floating_point). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this space is getting vacuumed?
I've cleaned up the generated code in the examples folder here: https://github.com/goldcaddy77/warthog/pull/289/files . Can you rebase? |
@@ -16,7 +16,7 @@ import { registerEnumType } from "type-graphql"; | |||
const { GraphQLJSONObject } = require("graphql-type-json"); | |||
|
|||
// @ts-ignore | |||
import { BaseWhereInput, JsonObject, PaginationArgs } from "../../../src"; | |||
import { BaseWhereInput, JsonObject, PaginationArgs } from "../../../packages/core/src"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@warthog/core
"url": "https://github.com/goldcaddy77/warthog/issues" | ||
}, | ||
"scripts": { | ||
"build": "rm -rf ./dist && tsc -p tsconfig.build.json && copyfiles -u 1 src/**/*.ejs dist/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the copyfiles piece - it's used in the CLI
@@ -73,7 +73,7 @@ export class Config { | |||
WARTHOG_DB_SUBSCRIBERS: ['src/subscribers/**/*.ts'], | |||
WARTHOG_DB_SUBSCRIBERS_DIR: 'src/subscribers', | |||
WARTHOG_DB_SYNCHRONIZE: 'false', | |||
WARTHOG_MODULE_IMPORT_PATH: 'warthog', | |||
WARTHOG_MODULE_IMPORT_PATH: '@warthog/core', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this option can be removed altogether since it's no longer dynamic. Does lerna + yarn workspaces allow you to pick up changes in @warthog/core
in real time as you make the changes so that they're usable in the example?
Just started reviewing :) |
Ok I made a bunch of updates here and put in a PR to your branch: thiagozf#1 . Can you get these in before you do more work? This should get things pretty close. Note that we should not put this into |
@thiagozf - did you see my PR into your branch? You should pull this in and it’ll get things pretty close. |
fix(monorepo): various monorepo fixes
Hey @goldcaddy77. Sorry for the late reply, last couple of days were tough. I plan get back to work on this next weekend. In the meantime, I've merged your PR. |
Great, thanks - looking forward to getting this merged. We should discuss whether this should be in an alpha branch for Warthog V3 since it will be a breaking change. |
Hey, I’ve merged version 2.0 and want to get another beta version for V3 with the monorepo changes. What is your availability like in the next few weeks? |
@thiagozf do you mind if I pick this work up where you left off once I start working on 3.0? |
Just started reviewing :) |
WIP: This is a work in progress!
Summary
This pull request is a proposal to split Warthog's codebase into
cli
,core
andserver-express
packages.Problem
I want to use Warthog's codegen and
Model
abstraction in a serverless environment - namely, inside Lambda functions. However, the framework is currently coupled with Express.js. There are alternatives, likeaws-serverless-express
, that allows me to use Express inside Lambda functions, but they don't feel right for brand new serverless projects.Current Warthog distribution also includes a ton of dependencies that don't actually need to be packaged and shipped to Lambda, because they are not used at runtime - eg.:
gluegun
forcli
/warthog
bin;dotenvi
bundle is huge and is only needed to load environment variables - which can be configured via CI. This unnecessarily increases the size of the package and makes the deploy/update much slower than it would be otherwise.Solution
If Warthog's code was split, one would be able to use only its
core
logic - eg.:Model
abstraction - and develop a custom server. The custom server could be a Lambda handler or a NestJS module, for example.To validate this idea, I've created three sub-packages:
@warthog/core
contains the framework's main logic (decorators and codegen), but it is now decoupled from Express@warthog/server-express
has all the Express server logic@warthog/cli
is the CLII am using
lerna
to manage the project's packages. I was able to successfully run a Lambda function powered by Warthog, with a custom server implementation, as a proof of concept. All existing tests are running fine.ToDo
Everything seems to be working fine, but there is still some work to be done:
BooleanField
andJsonField
) and examples (need to validate if it was the monorepo migration that caused it)With that being said, a monorepo structure is definitely challenging to manage and I'm not sure if this is what you guys want for this project. I would be happy to make any changes you deem necessary and submit a final pull request depending of your feedback.
Oh, and thanks for this awesome piece of work that is Warthog 😊
Closes #147