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

Add Directory.Packages.props #291

Merged
merged 3 commits into from
Dec 14, 2023
Merged

Add Directory.Packages.props #291

merged 3 commits into from
Dec 14, 2023

Conversation

Xeinaemm
Copy link
Contributor

@Xeinaemm Xeinaemm commented Dec 11, 2023

Changes:

  • Migrated NuGet packages to a central place in Directory.Packages.props. It supports UI updates in comparison to old MSBuild solutions. This solution doesn't use Central Package Management (CPM) flag, only MSBuild NuGet feature. We have a standardized layout of projects so it doesn't make sense to store versions in one place and references in projects. Credits to @AlmarAubel for suggestions.
  • Removed ProjectReferences from all projects. Directory.Build.targets is more readable because now represents folder structures and particular sets of projects. Changes the best to read in split mode.

This PR closes MSBuild changes, there are no hardcoded values and it fully works based on patterns. For example, by adding a new module it will automatically set all necessary dependencies for the new module and also add it to API. IntegrationEvents works similarly, it will automatically add new project events to other modules' applications.

The only manual thing to do is to add a new module and include all the projects in the solution, but this can be fixed by creating a module template via .NET Templates.

@kgrzybek
Copy link
Owner

Hi @Xeinaemm

First of all - thanks for changes!

  1. Using Directory.Packages.props it is great thing but this is a tradeoff - we lose modules autonomy and modularization. This way you cannot upgrade package only in one module without changing other modules (override). I mean sometimes it is not a problem, but with larger projects with multiple teams working on the same monolith could be a problem. I accept this change but I will write ADR or some info in README about this considerations.

  2. Autoreferencing libraries based on convention is good, but there is one exception. Before the change we exactly know which integration events were used by which module. This was important. After the change all modules reference all integration events and this is not good (for instance UsersModule referenced only Meetings.IntegrationEvents, now it references all IntegrationEvents from all modules. So imo we should not do it for integration events and keep them as before (explicit reference).

@Xeinaemm
Copy link
Contributor Author

Xeinaemm commented Dec 12, 2023

Using Directory.Packages.props it is great thing but this is a tradeoff - we lose modules autonomy and modularization.

It's true with CPM enabled, but I avoided this feature in this case. I managed 150+ shared libraries and 120+ services which were put into 32 products similar to this template. I made this in a way that you can always add your libraries and override them freely in any project. Only downgrading defined dependencies isn't allowed. The goal of Directory.Packages.props should be a source of truth, it's easier for me as a maintainer to look at one place and update a dozen products at once. In the end, you have a finite number of dependencies, even after 20 years of development.

I can help you with ADR, but in this case, I didn't want to change behaviors, only places. I have for sure some objections to some dependencies or references but I need to put it in words.

So imo we should not do it for integration events and keep them as before (explicit reference).

Ok

@kgrzybek kgrzybek merged commit 107e81f into kgrzybek:master Dec 14, 2023
2 checks passed
@kgrzybek
Copy link
Owner

@Xeinaemm good job!

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