-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
chore: add /generator path #7
chore: add /generator path #7
Conversation
456d081
to
f288a7a
Compare
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.
A few comments 🙂 Looking good 👍
src/middlewares/generator-template-parameters-validation.middleware.ts
Outdated
Show resolved
Hide resolved
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 👍
Yeah, I'd not go with REST design for this API. Pure RPC seems a better fit IMHO. All methods will be POST and paths will always be verbs. The rest of the info can be provided in the request payload. Like you said, @magicmatatjahu, just like a CLI. |
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'd just change the path to /generate
. Other than that, looks good to me.
headers: { | ||
Cookie: req.header('Cookie'), | ||
}, | ||
withCredentials: 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.
How this is gonna work? What are the credentials? Are they gonna be based on a cookie? I'm confused.
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.
It's used for parser options and it's exactly options for json-schema-ref-parser
. More info here :) https://github.com/APIDevTools/json-schema-ref-parser/blob/main/docs/options.md#resolve-options In this same way we do in old playground :)
src/middlewares/generator-template-parameters-validation.middleware.ts
Outdated
Show resolved
Hide resolved
req.parsedDocument, | ||
template, | ||
parameters, | ||
tmpDir, |
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 still think we should move into generating in memory, writing directly to the zip stream by using (If I'm correct) append
archiver method: https://www.archiverjs.com/docs/archiver#append.
The whole process will be generated on the fly, way faster, and theoretically, since you piped the stream to the response object, data should be flushed on each write, isn't? Meaning you don't hold everything in memory but just a small buffer since everything is drained though the response (sent back to the client).
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.
@smoya Maybe I'm not understanding it correctly, but how to do that if generator uses under the hood filesystem api to "render" templates? For that I passed directory destination for output.
I just want to drop a comment regarding the whole design pattern used on this app. We are currently following some sort of MVC pattern, which it is enough for simple applications IMHO (like this one today). However, I can detect and I would say even foresee that the logic on this API will be very similar, in part, to what the CLI does nowadays. |
1 similar comment
I just want to drop a comment regarding the whole design pattern used on this app. We are currently following some sort of MVC pattern, which it is enough for simple applications IMHO (like this one today). However, I can detect and I would say even foresee that the logic on this API will be very similar, in part, to what the CLI does nowadays. |
@smoya I totally agree on moving to another architecture if you foresee most logic will be reusable on this API and the CLI 👍. Do you think it's worth chasing a move towards this architecture now, or keep it simple until we find the need to change the architecture (I'm kinda debating whether to follow the KISS principle or do more upfront design 😅)? Could you share briefly what sort of areas you see being reused? I'm asking because I had a look at this PR and the CLI repo, but couldn't find the type of logic that this API would use (I mostly looked at commands and models). |
@smoya @BOLT04 Thanks for review and for feedback! Honestly, about hexagonal architecture, I don't think so that it will be good idea. CLI (as @BOLT04 noted) has models and command, server-api will have controllers/routes (and some services). Even if we wanna create reusable parts (like services) we should see that for CLI/Server-API given service can be significantly different for two given environments. For example the generator - on the server side we have to generate in temporary folders and send the zip, in the CLI we generate in the folder specified by the user. I can say that currently we are using quasi-hexagonal pattern due to the fact that every tool in our organization is separate library and can be easily integrated into every environment. Why make life difficult for yourself, make wrappers/adapters in one, shared place and then again in another place, whether it is a server or CLI? 🤷🏼 |
Maybe its time to change the generator :). Anyway, I will move this into separate issues so we are not completely blocking this pr. |
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.
There are a couple of concerns I will like to resolve asap. However, im gonna approve this PR so we can discuss those deeply and move forward without blocking.
🚀 Nice job @magicmatatjahu !!!💯
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 👍
@smoya could you add the link for the issue afterward here? I'd like to follow that discussion 🙂 |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
I see some code smells from SonarCloud. I will merge that PR with |
@magicmatatjahu @BOLT04 I created a discussion about this topic with a more expanded context. You might want to look at it asyncapi/community#212 |
Description
Add
/generator
path:Initialize
/src
structure:(logic for route and their initialization)
, services (business logic and helpers)ProblemException
errorOpenAPI document:
html
andmarkdown
, now@asyncapi/html-template
and@asyncapi/markdown-template
Problem
schemaSome insights:
I think that we should go with names of paths with verbs like in our CLI, so
/generator
should be/generate
, in future/validate
,/convert
,/bundle
etc. I know that we decided go with REST patterns, but it doesn't work, really, I always wrote (in tests and source code)/generate
path rather than/generator
so it's more natural.cc: @smoya @jonaslagoni @fmvilas @derberg
If you want share your opinions @BOLT04 I would be grateful, but if you don't have time, it's okay :)