Skip to content
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

Feature: IoC container #9

Merged
merged 8 commits into from
Jul 1, 2022
Merged

Feature: IoC container #9

merged 8 commits into from
Jul 1, 2022

Conversation

oscar60310
Copy link
Contributor

@oscar60310 oscar60310 commented May 16, 2022

This PR added Inversify as our inversion of control (IoC) container, followed by some necessary changes.

  1. IoC containers.
    • src/containers - TYPES symbol list, wrapped container, and modules.
    • src/options - Opions classes
  2. Path alias
    All path alias have been moved to root tsconfig. But there will be some issues after building projects.
  3. Use a facade class instead of pipeline module

@oscar60310 oscar60310 changed the base branch from develop to feature/extensions-and-filters May 16, 2022 10:35
@oscar60310 oscar60310 force-pushed the feature/ioc-container branch 3 times, most recently from 126be90 to 0d93124 Compare May 20, 2022 03:21
@oscar60310 oscar60310 changed the title [WIP] Feature: IoC container Feature: IoC container May 20, 2022
@oscar60310 oscar60310 marked this pull request as ready for review May 20, 2022 04:08
@oscar60310 oscar60310 force-pushed the feature/extensions-and-filters branch from b0aeb03 to 4ec7870 Compare June 23, 2022 09:27
Base automatically changed from feature/extensions-and-filters to develop June 28, 2022 03:06
Copy link
Contributor

@kokokuo kokokuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great for using IoC, besides some questions and suggestion, others LGTM 👍

@@ -7,7 +7,7 @@ export interface RawRequestParameter
}

export interface RawAPISchema extends DeepPartial<Omit<APISchema, 'request'>> {
name: string;
sourceName: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding the comment to make the members know the source name is the schema file name or rename it to schemaFileName or fileName ?

Because sourceName may not easy to know where the source from, even we could know the templateSource will be assigned the source name if not foundtemplateSource in getTemapleteSource middleware, we still do not know where the default source name from.

And also, even though we found the sourceName is from name field in the SchemaData, the name is a general word ( maybe we could also rename the name in the SchemaData or just also add a comment ) until we see the code of readSchema in the FileSchemaReader, however, it may spend some time to trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed these two properties to "sourceName" and added some comments about them.

export interface RawAPISchema extends DeepPartial<Omit<APISchema, 'request'>> {
  /** Indicate the identifier of this schema from the source, it might be uuid, file path, url ...etc, depend on the provider */
  sourceName: string;
  request?: DeepPartial<RawRequestParameter[]>;
}
export interface SchemaData {
  /** The identifier of this schema, we might use this name to mapping SQL sources. */
  sourceName: string;
  content: string;
  type: SchemaFormat;
}

The sourceName here indicated the identifier of the schema, it might be file path (File provider), URL (s3 provider), UUID (database provider) ...etc. in the future, so I didn't rename it the schemaFileName or fileName.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for telling me the sourceName purpose in the future, thanks, no problem and thanks for you comment added 👍

@@ -0,0 +1,8 @@
export interface ISchemaParserOptions {
reader: SchemaReaderType;
schemaPath: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why rename to schemaPath ?
Actually I think folderPath is good because the folderPath could let us know the path is the folder, however, the schemaPath may not know the path is a file or folder until we saw the code in FileSchemaReader .

If you would like to let member know the path is the shcema folder path, maybe you could just name schemaFolderPath (although it may be a little long, but could be clear I think, how do think ? )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both schemaPath and templatePath have been renamed to folderPath, thanks!

@kokokuo
Copy link
Contributor

kokokuo commented Jun 28, 2022

Also please rebase develop branch, thanks ~

@oscar60310 oscar60310 force-pushed the feature/ioc-container branch from 01687ba to 1c5e76f Compare June 30, 2022 07:33
@oscar60310
Copy link
Contributor Author

Hi @kokokuo , I've rebased the branch and fixed the issues.

@kokokuo
Copy link
Contributor

kokokuo commented Jul 1, 2022

It has been checked, No Problem, thanks for replying to me 👍

@kokokuo kokokuo merged commit 50246e3 into develop Jul 1, 2022
@kokokuo kokokuo deleted the feature/ioc-container branch July 1, 2022 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants