-
Notifications
You must be signed in to change notification settings - Fork 11
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
Refactor Wrapper to Injected Service (WrapperFactory) #536
Conversation
I'm (almost) always pro DI, so go for it. |
Hard to evaluate because of the huge changeset, but in general I am also happy with it. I'd question why you have a |
I'd say the design becomes a little more complex and more rigid (as you can't just access the wrappers from anywhere).
That stuff comes with Langium. It think this is a "poor man's version of DI". Instead of passing the dependent objects to the constructor, you pass all available objects via a DI container and let the constructor choose what it needs... |
Ja, sounds fair. If it is actually the design langium suggests, we should capture that in a comment (with a link as you did here) in the JayveeAddedServices class. |
Otherwise I am happy with this. |
const operatorEvaluatorRegistry = new DefaultOperatorEvaluatorRegistry(); | ||
|
||
const executionContext = new ExecutionContext( | ||
pipeline, | ||
new TestExecExtension(), | ||
new DefaultConstraintExtension(), | ||
new CachedLogger(runOptions.isDebugMode, undefined, loggerPrintLogs), | ||
new WrapperFactory(operatorEvaluatorRegistry), | ||
runOptions, | ||
new EvaluationContext(new RuntimeParameterProvider()), | ||
new EvaluationContext( | ||
new RuntimeParameterProvider(), | ||
operatorEvaluatorRegistry, | ||
), |
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.
This code snippet exists at a plethora of places. Can we move to a method?
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.
Done in 8db76f0.
libs/language-server/src/lib/ast/expressions/operator-evaluator.ts
Outdated
Show resolved
Hide resolved
I decided to explicitly not cover |
For a code review, I recommend to first look into the following changes before skimming over the import and parameter changes:
Most representative commits to understand what happened chronologically:
|
In the process of #427, I noticed there are some dependency cycles that cannot be resolved with ESM modules.
This PR explores whether we can remove some dependency cycles by using the dependency injection mechanism of Langium. Instead of using exported instances for singletons, I registered the following services:
In terms of specific dependency cycles, this PR tackles cycles when the instances
unaryOperatorRegistry
,binaryOperatorRegistry
,ternaryOperatorRegistry
were imported. Instead, the service interface is injected, so the transpiler only needs to import the type and not the instance.