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

Small refactor for some repositories calling other repositories #212

Open
wants to merge 1 commit into
base: feature/customize-logo-greeting
Choose a base branch
from

Conversation

p3rcypj
Copy link
Contributor

@p3rcypj p3rcypj commented Feb 5, 2025

📌 References

📝 Implementation

Some repositories are called by other repositories, which is a code smell. For those were we were just for the intention to get the api, I pass the api instead of the repository.

@p3rcypj p3rcypj requested a review from tokland February 5, 2025 10:14
@p3rcypj p3rcypj mentioned this pull request Feb 5, 2025
2 tasks
Copy link
Contributor

@tokland tokland left a comment

Choose a reason for hiding this comment

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

[code-only review]

All good. Just one in-line comment regarding D2Api building. As that's a trivial thing, I am approving already. Request re-review if you do some more complex refactor.

const trainingModuleRepository = new TrainingModuleDefaultRepository(configRepository, instanceRepository);
const landingPageRepository = new LandingPageDefaultRepository(configRepository, instanceRepository);
const documentRepository = new Dhis2DocumentRepository(instance);
export function getCompositionRoot(api: D2Api) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the idea for the baseUrl -> api transition? Typically we'd like to pass the less specific object (baseUrl) and let the composition root how to build D2Api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I did it because it was the way we were doing it in the skeleton now, but as you mentioned it, it makes sense to give the baseUrl as the param and let the compositionRoot build the D2Api

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Yes, we follow this approach in the skeleton, but mainly for historical reasons—index.tsx originally handled building the old d2 object. However, that's not ideal. A D2Api is a 'data' concern, whereas baseUrl is a 'webapp' concern. So, let's pass the least compromising option instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants