-
Notifications
You must be signed in to change notification settings - Fork 45
Feature: refactor to IoC (inverse of control) pattern in serve package #18
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
Conversation
… api server from api schema - add serve package by "nx generate" command - add "DateTypeValidator", "IntegerTypeValidator", "StringTypeValidator", "UUIDTypeValidator" and its test cases. - add "loadedValidators" for loading all built-in and user-defined validator dynamically. - install "joi", "dayjs", "faker", "ts-sinon", "uuid" plugins at vulcan "package.json". - add "ReuqestTransformer", "RequestValidator", for converting request data format and validating. - add "BaseRoute", "RestfulRoute", "GraphQLRoute" to create route. - add "RouteGenerator" to generate Route according api route type. - add "VulcanApplication" to add routes and create server. - add test cases for "ReuqestTransformer", "RequestValidator", "RouteGenerator" and "VulcanApplication" - install "suepertest" in package.json for testing koa server. - add "start" command to run serve package directly for testing in project.json. - create "normalizedStringValue" module to handle string value. - add "../../types/*.d.ts" in tsconfig of serve.
- add "DataQueryBuilder" to provide select, join, where, group, having, order, limit, offset by query chain - add "JoinOnClause" to provide join-on methods - add test cases of "DataQueryBuilder" providing select, join, where, group, having, order, limit, offset clauses. - add test cases of "JoinOnClause" provding on clauses. - refactor "baseRoute" for passing template engine and use template engine. - fix test cases of "RouteGenerator" and "VulcanApplication".
…a query builder - add pagination field in "APISchema". - add "PaginationStrategy" interface and offset, cursor, keyset-based strategy class. - add "PaginationTransformer" to do transfrom from koa ctx . - add data source interface "IDataSource". - refactor "BaseRoute" for splitting request and pagination transformer to implemented restful & graphql class, because of graphql get request from the body. - update test cases of each sql clause method of "DataQueryBuilder".
…ame. - add "ValidatorLoader" to get validator according load validator by name. - add "RequiredValidator" to check value is required. - refactor to export default for "DataTypeValidator", "IntegerTypeValidator", "UUIDTypeValidator", "StringTypeValidator". - refactor "RequestValidator" to validate data by using "ValidatorLoader" - update related test cases in core and serve package.
…n middlewares and middleware loader - add "tslog" package and create "getLogger" by "LoggerFactory". - fix "transform" method in "PaginationTransformer". - add "defaultImport" function for loader used. - remove "default" for each validator classes and add "index.ts" in "data-type-validators" for export default. - add built-in middlewares for cors, request Id, audit logging, rate limit, and add test cases - add the middleware loader to load the middleware module. - set middleware and add test cases in the vulcan app server. - add ignore tag in "IPTypeValidator" class in test/ to prevent detecting unnecessary code.
2e9d1c1 to
ca40da2
Compare
|
rebase finished, start to refactor for using IoC pattern. |
…ule import logistic, add required validator. - Rename folder from "data-type-validators" to "built-in-validators" - Add "required" validator test cases - refactor "validateData" method to any in "IValidator". - refactor validateData type for "IntegerTypeValidator", "RequiredValidator". - add "extensions" property in "ICoreOptions" to collect different extension modules name / folder path. - add ""SourceOfExtensions" TYPE and refactor to inject extensions in "ValidatorLoader" - refactor "defaultImport" to import multiple modules - add "mergedModules" to merge multiple extension modules to the one module object. - change to container load and enhance test cases of validator loader to check importing multiple modules. - add "setupFilesAfterEnv" in "jest.config.ts" import "reflect-metadata" first for jest testing could collect IoC inject decorators.
…tensions" property in test cases
… to core package. - create "pagination.ts" in core package to define pagination model. - move "data-query" folder and "data-source" folder to core package.
…C conatiner. - update the "exectueModule" to bind real data query builder. - update data query builder to return by "IDataQueryBuilder". - update test case for test compiler.
- refactor import way from "'fs/promises'" to "{ promises as fs } from 'fs'" for making jest debug work.
- fix the "ValidatorLoader" inject optional field in constructor.
- add "TYPES", "Container" and load "routeGeneratorModule".
- refactor "ServeConfig" for inheriting "CoreOptions" and create "AppConfig" for middleware and "VulcanApplication".
- fix "loadExtensions" function to merged modules after "defaultImport" response updated.
- refactor "VulcanApplication" for focusing on build middleware and route.
- create "VulcanServer" for running server.
- update all test cases for supporting IoC pattern in serve package.
|
Update all serve packages to use IoC Pattern and create by IoC Container, all test cases passed (core, build, serve). |
oscar60310
left a comment
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.
LGTM besides some questions about IoC.
| } | ||
|
|
||
| const validator = loader.getLoader(validatorRequest.name); | ||
| const validator = await loader.load(validatorRequest.name); |
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.
NIT: I'll call getLocal multiple times, it appeared to me that we have imported the code of validators to memory, we just initialized them here. But with your code, we seem to import the code via .load function, will it cause performance issues?
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.
Thanks @oscar60310 for reviewing and suggesting, after discussing in the morning, this part will keep and do it after a meeting discussion.
| // Data Query Builder | ||
| IDataQueryBuilder: Symbol.for('IDataQueryBuilder'), | ||
| // Data Source | ||
| IDataSource: Symbol.for('IDataSource'), | ||
| // Validator | ||
| ValidatorLoader: Symbol.for('ValidatorLoader'), | ||
| IValidatorLoader: Symbol.for('IValidatorLoader'), |
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 remember we've discussed this, but I still have some questions:
Why did we use the leading "I" for the identifiers? It makes sense to use "I" to indicate the difference between interface and class like this:
interface Ixxxx {}
class xxxx implement Ixxxx {}But here we don't care about what type we'll get, we just want an instance, it might confuse us if we don't use a consistent naming rule, that is, TYPE.Executor but TYPE.IDataSource.
Did you want to let the identifiers have the same name as their interface?
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.
Thanks @oscar60310 for reviewing and suggesting, after discussion, we keep the TYPES no need to add prefix I and make the interface add the prefix I for those needed to be implemented by the class.
packages/build/test/schema-parser/middleware/transformValidator.spec.ts
Outdated
Show resolved
Hide resolved
| createBuilder: async (query: string) => | ||
| new DataQueryBuilder({ statement: query, dataSource }), | ||
| }; |
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.
NIT: How about creating an Executor class instead of using a pure object?
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.
Thanks @oscar60310 for reviewing, I have created a pure class QueryExecutor to do it.
6a195f3 to
553671e
Compare
…te property in multiple module. - remove the logistic for throwing duplicate property in multiple module in "mergedModules". - add the "checkSameNameValidator" logistic for checking duplicate key name after creating valicator by class from module. - add the logistic for checking duplicate key name after creating middleware by class from module in "VulcanApplication". - add more test cases for validator loader to checking load extensions. - move the "setupFilesAfterEnv" setting to "jest.preset.ts" - rename for removing prefix "I" in symbol in the "types.ts" from core and package. - add "QueryExecutor" to create data query builder.
553671e to
fa3d166
Compare
|
Hi @oscar60310 all suggestion has been fixed, please check it, thanks too much. |
oscar60310
left a comment
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.
LGTM 👍
Description
Rebase develop and adapt IoC pattern to inject dependencies by
InversifyJSfor theservepackage. The PR includes the previous implemented and approved features from PRs:After rebased, and Refactor to the IoC pattern, there are some things changed:
all packages
setupFilesAfterEnvinjest.config.tsand importreflect-metadatafirst for jest testing could collect IoC inject in each package.In the core package
defaultImportlogistic to support importing multiple extensions and addmergedModulesto merge multiple extension modules to the one module object.pagination.tsincorepackage to definepaginationmodel and movedata-queryfolder anddata-sourcefolder tocorepackage ( also include test cases ), becausebuildandservealso need them.executorModuleto bind the real data query builder and update the test case for the test compiler.In the serve package
ServeConfigfor inheritingCoreOptionsand createAppConfigfor middleware andVulcanApplication.loadExtensionsfunction to merged modules afterdefaultImportresponse updated.VulcanApplicationfor focusing on building middleware and route and createVulcanServerfor running the server.How To Test / Expected Results
For the test result, please see the below test cases that passed the unit test:
The core package
The build package
The serve package
Commit Message
Because the PR included rebased commits, here is only the list refactor to IoC commits:
data-type-validatorstobuilt-in-validators.requiredvalidator test casesvalidateDatamethod to any inIValidator.validateDatatype forIntegerTypeValidator,RequiredValidator.extensionsproperty inICoreOptionsto collect different extension modules name / folder path.SourceOfExtensionsTYPE and refactor to inject extensions inValidatorLoader.defaultImportto import multiple modulesmergedModulesto merge multiple extension modules to the one module object.setupFilesAfterEnvinjest.config.tsimportreflect-metadatafirst for jest testing could collect IoC inject decorators.decorators.setupFilesAfterEnvinjest.config.js, supportextensionsproperty in test cases.corepackage.pagination.tsincorepackage to define the pagination model.data-queryfolder anddata-sourcefolder tocorepackage.execute moduleto bind the real data query builder.IDataQueryBuilder.servepackage by IoC pattern.fs/promisesto{ promises as fs } from 'fs'for making jest debug work.ValidatorLoaderinject optional field in the constructor.TYPES, "Container" and loadrouteGeneratorModule.ServeConfigfor inheritingCoreOptionsand createAppConfigfor middleware andVulcanApplication.loadExtensionsfunction to merged modules afterdefaultImportresponse updated.VulcanApplicationfor focusing on building middleware and route.VulcanServerfor running the server.servepackage.mergedModules.checkSameNameValidatorlogistic for checking duplicate key name after creating validator by class from the module.VulcanApplication.setupFilesAfterEnvsetting tojest.preset.ts.