-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
feat!: add dependency manager #947
feat!: add dependency manager #947
Conversation
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.
For me these solution can be, but the AbstractDependencyManager
itself should have a method that renders all dependencies as output like renderDependencies()
. In addition, a better solution would be if we could pass the dependency manager class as generator option (definition of it) and create it during rendering - currently someone can pass this via an argument to the method, and it should be passed to the constructor option.
I think that makes sense, let me try to look into that!
I don't quite understand this suggestion, can you clarify it? |
For example. you have signature: public abstract render(model: MetaModel, inputModel: InputMetaModel, dependencyManager?: AbstractDependencyManager): Promise<RenderOutput>; I think that's would be better if people would pass {
dependencyManager?: typeof AbstractDependencyManager // definition of class not instance!
} and then if someone will run render(...) {
const dependencyManager = new options.dependencyManager(...)
} Is it clarify? :D |
Yes! Actually, that's brilliant! Let me try out that approach 😄 |
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.
Few comments :)
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.
One thing, we discussed about it as I remember 😅
# Conflicts: # examples/README.md # src/generators/typescript/TypeScriptGenerator.ts # src/generators/typescript/TypeScriptRenderer.ts # src/helpers/ConstrainHelpers.ts # src/helpers/TypeHelpers.ts
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 like it. A couple of things I don't like is that you add too many things through class arguments and not through some single object, such as here https://github.com/asyncapi/modelina/pull/947/files#diff-a6a5c62c72e934883df999f53c7335c2b386754b04ca5222520015351537d546R20 but it's ok 👌🏼
Co-authored-by: Maciej Urbańczyk <urbanczyk.maciej.95@gmail.com>
@magicmatatjahu lets merge it into a separate branch to allow each generator and maintainer to review the changes more easily. |
Kudos, SonarCloud Quality Gate passed! |
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.
@jonaslagoni Yeah, good idea. If new feature has a bugs that I don't see, it will be easier to fix :)
Description
Up until now, dependencies have been part of the renderer instance as it was only the presets and renderers that needed to add dependencies, however, there is a need to let the type constrainer add dependencies. This creates a bit of a dilemma, as the renderer expects a constrained model, and to create a constrained model it needs a renderer...
So therefore I suggest we split out the dependency functionality into its own instance where each language can create its own custom logic for handling dependencies. For example, TypeScript needs specific functions to more efficiently make imports based on the options from the user.
I would love to have alternatives, and if you have something please let me know, cause it does create somewhat of a complexity 🙃 But maybe this is for version 2? 🤷
Alternatives I considered:
Changes introduced:
Related issue(s)
Fixes #851