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

Refactor Wrapper to Injected Service (WrapperFactory) #536

Merged
merged 24 commits into from
Apr 5, 2024

Conversation

georg-schwarz
Copy link
Member

@georg-schwarz georg-schwarz commented Apr 2, 2024

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:

export interface JayveeAddedServices {
  // ...
  operators: {
    TypeComputerRegistry: OperatorTypeComputerRegistry;
    EvaluatorRegistry: OperatorEvaluatorRegistry;
  };
  WrapperFactory: WrapperFactory;
  // ...

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.

@georg-schwarz
Copy link
Member Author

@rhazn @joluj Is this a refactoring that you would welcome?

If yes, I'd go ahead and proceed with all the remaining wrappers as well.

@joluj
Copy link
Contributor

joluj commented Apr 2, 2024

I'm (almost) always pro DI, so go for it.

@rhazn
Copy link
Contributor

rhazn commented Apr 2, 2024

Hard to evaluate because of the huge changeset, but in general I am also happy with it. I'd question why you have a JayveeAddedServices object and not just use the DI for each service independently (what does bundling them all into one object accomplish?). But regardless what the answer to that question is, I am happy with your choice ;).

@georg-schwarz
Copy link
Member Author

Hard to evaluate because of the huge changeset, but in general I am also happy with it.

I'd say the design becomes a little more complex and more rigid (as you can't just access the wrappers from anywhere).

I'd question why you have a JayveeAddedServices object and not just use the DI for each service independently (what does bundling them all into one object accomplish?).

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...
The JayveeAddedServices is a type we union with the DI container that comes with Langium. For more info, see here.
I'd just go with that design.

@rhazn
Copy link
Contributor

rhazn commented Apr 2, 2024

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.

@rhazn
Copy link
Contributor

rhazn commented Apr 2, 2024

Otherwise I am happy with this.

Comment on lines 60 to 72
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,
),
Copy link
Member Author

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 8db76f0.

@georg-schwarz
Copy link
Member Author

I decided to explicitly not cover ValueTypes in this PR not further to bloat up the PR.
Refactoring them accordingly should be subject to a future PR, that is hopefully able to resolve some more dependency cycles.

@georg-schwarz georg-schwarz marked this pull request as ready for review April 4, 2024 09:02
@georg-schwarz georg-schwarz changed the title WIP: Refactor Wrapper to Injection Refactor Wrapper to Injected Service (WrapperFactory) Apr 4, 2024
@georg-schwarz
Copy link
Member Author

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:

  1. Break up some dependencies in the expression directory - ea89232
  2. Inject evaluator and type computer registry as service - e593726
  3. Introduce WrapperFactory for creating BlockTypes and TypedObjects - 15a1213
  4. Envorce WrapperFactory over BlocktypeWrapper and ConstraintTypeWrapper - 93e824f
  5. Simplify validation methods by introducing JayveeValidationProps as generalized parameter - 8db76f0

@georg-schwarz georg-schwarz merged commit b851b83 into main Apr 5, 2024
3 checks passed
@georg-schwarz georg-schwarz deleted the dependency-cycles branch April 5, 2024 13:40
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants