-
Notifications
You must be signed in to change notification settings - Fork 45
Feature: Implement building components #1
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
84139a4 to
866ee1c
Compare
66e5b7b to
9f60ab8
Compare
| schemas.request[requestIndex].validators = | ||
| schemas.request[requestIndex].validators?.map((validator) => { | ||
| // 1. some-name -> {name: "some-name"} | ||
| if (typeof validator === 'string') { |
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.
Great handling! Thanks for normalizing the schema validator format by handing the some-name -> {name: "some-name"}
| export const generateTemplateSource = | ||
| (): SchemaParserMiddleware => async (schemas, next) => { | ||
| if (schemas.templateSource) return next(); | ||
| schemas.templateSource = schemas.sourceName; |
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.
Maybe here you could add a comment to let others know you give the schema file name by default if the template source is not set?
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.
Sure, added.
| @@ -0,0 +1,13 @@ | |||
| export enum SchemaDataType { | |||
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.
How about SchemaDataFormat or SchemaFormat or SchemaFormatType ?
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.
SchemaFormat sounds greate :_)
| @@ -0,0 +1,5 @@ | |||
| export interface Compiler { | |||
| name: string; | |||
| compile(template: string): string; | |||
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.
Suggest adding comment to describe what the result is and its purpose, e.g: object code, or intermediate, because these parts may not easy to understand for maintainer in the future.
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've added some comments here, but I'm afraid it won't help a lot. Because the compiler logic should be defined in child classes, for example, Nunjucks compiler turns the template to JavaScript code, but another compiler might compile some binary data. I can't have a clear explanation here.
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.
Got it, you're right it not easy to explain if we'll provide other compliers in the future.
| if (compiledResult) { | ||
| Object.keys(compiledResult.templates).forEach((templateName) => { | ||
| loader.setSource(templateName, compiledResult.templates[templateName]); | ||
| }); | ||
| } | ||
| const compiler = new NunjucksCompiler({ loader }); |
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.
Great coding! and I have a question about compiler name:
I saw you record the compiler name (e.g: nunjucks ) in the PreCompiledResult and save the result with template and schema in file through ArtifactBuilder, but since did not according to the compiler name to switch compiler when using useDefaultLoader to create TemplateEngine and render the result.
Because I consider saving the compiler name in builder file is ordering to read different compiler when rendering through TemplateEngine by useDefaultLoader .
So, should we still record the compiler name in PreCompiledResult when compiling through TemplateEngine ?
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.
Because I consider saving the compiler name in builder file is ordering to read different compiler when rendering through TemplateEngine by useDefaultLoader .
Yes, you were right. I thought we might have different compilers in the future. Saving the compiler name will let us indicate the different compilers.
But after implementing further features, I think we have little chance to provide multiple compilers in the same time because the extensions are coupled with compiler. So I removed the compiler name in comilerReulst, thanks!
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 for sharing more possible plans in your mind to let me know ~
kokokuo
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.
Greate implementing! LTGM 👍
Keep some suggestion for commenting and question.
Add build pipeline for demonstrating template engine, schema parser, and artifact builder. Rename all test file to *.spec.ts. Move definition files to root folder.
35b57dd to
b4fb270
Compare
|
@kokokuo Thanks for reviewing, all issues were solved, cc @oscar60310 . |
|
Checked, LGTM 👍 please feel free to merge. |
This PR includes the MVP of three components: Template engine, schema parser, and artifact builder:

I have also added some test cases to demonstrate the usage of the whole build pipe located at /build/test/pipeline.
This artifacts will be like below:
{ "schemas": [ { "sourceName": "detail/role", "name": "Role", "url": "/detail/role/:id", "request": [ { "name": "id", "in": "query", "description": "role id", "validators": [ { "name": "uuid", "args": {} }, { "name": "required", "args": {} } ] } ], "urlPath": "/detail/role", "templateSource": "detail/role" }, { "sourceName": "user", "name": "User", "url": "/user/:id", "request": [ { "name": "id", "in": "query", "description": "user id", "validators": [ { "name": "uuid", "args": {} }, { "name": "required", "args": {} } ] } ], "error": [ { "code": "Forbidden", "message": "You are not allowed to access this resource" } ], "urlPath": "/user", "templateSource": "user" } ], "compiler": "nunjucks", "templates": { "detail/role": "(() => {function root(env, context, frame, runtime, cb) {\nvar lineno = 0;\nvar colno = 0;\nvar output = \"\";\ntry {\nvar parentTemplate = null;\noutput += \"select * from public.role\\nwhere id = \";\noutput += runtime.suppressValue(runtime.memberLookup((runtime.memberLookup((runtime.contextOrFrameLookup(context, frame, \"context\")),\"params\")),\"id\"), env.opts.autoescape);\nif(parentTemplate) {\nparentTemplate.rootRenderFunc(env, context, frame, runtime, cb);\n} else {\ncb(null, output);\n}\n;\n} catch (e) {\n cb(runtime.handleError(e, lineno, colno));\n}\n}\nreturn {\nroot: root\n};\n})()", "user": "(() => {function root(env, context, frame, runtime, cb) {\nvar lineno = 0;\nvar colno = 0;\nvar output = \"\";\ntry {\nvar parentTemplate = null;\noutput += \"select * from public.user where id = '\";\noutput += runtime.suppressValue(runtime.memberLookup((runtime.memberLookup((runtime.contextOrFrameLookup(context, frame, \"context\")),\"params\")),\"id\"), env.opts.autoescape);\noutput += \"';\";\nif(parentTemplate) {\nparentTemplate.rootRenderFunc(env, context, frame, runtime, cb);\n} else {\ncb(null, output);\n}\n;\n} catch (e) {\n cb(runtime.handleError(e, lineno, colno));\n}\n}\nreturn {\nroot: root\n};\n})()" } }