search

kgrzybek / modular-monolith-with-ddd

Full Modular Monolith application with Domain-Driven Design approach.
MIT License
10.82k stars 1.7k forks source link

Upgrade to net 8 #286

Closed bistok closed 8 months ago

bistok commented 8 months ago

Here is the Pull Request for net 8

kgrzybek commented 8 months ago

Big PR, @bistok! :)

I will check it, please resolve conflicts before :)

bistok commented 8 months ago

Yes there where many places that needed changes, let me check the conflicts

bistok commented 8 months ago

Fixed conflicts. But made 2 mistakes on the merge, currently fixing it and compiling

bistok commented 8 months ago

Done all changes, Is ready for prime time, before the commit Style cop commit ran all test to validate everything was passing

AlmarAubel commented 8 months ago

Perhaps it's not my place to comment, but I believe that moving ICommand and IQuery to buildingblocks project may not be advisable. This is because you now lose IDE/build support for determining which commands and queries are permissible in each module, a feature that was previously available.

Previously you got an error like this 

Argument type 'CompanyName.MyMeetings.Modules.Meetings.Application.MeetingComments.AddMeetingComment.AddMeetingComment
Command' is not assignable to parameter type 'CompanyName.MyMeetings.Modules.Administration.Application.Contracts.ICommand'
kgrzybek commented 8 months ago

Perhaps it's not my place to comment, but I believe that moving ICommand and IQuery to buildingblocks project may not be advisable. This is because you now lose IDE/build support for determining which commands and queries are permissible in each module, a feature that was previously available.

It is definitely good place to comment, @AlmarAubel and this is valid point.

@bistok Can you revert this particular change?

bistok commented 8 months ago

@AlmarAubel and @kgrzybek, sure can be reverted, but the issue is that this fixes an Autofac IoC bug for discovering the Commands for the decorators. I can test more but that was a way I found for it to works well and because every project reference the buildingblocks that is like a SharedKernel, there where a good way not to repeat 4 times the ICommand and IQuery on each project see comments in PR https://github.com/kgrzybek/modular-monolith-with-ddd/pull/75

@AlmarAubel and is a perfect place to raise this concern, this is the exact place.

kgrzybek commented 8 months ago

@bistok I am checking your changes but general problem with .NET 8.0 is that is not supported by Rider https://rider-support.jetbrains.com/hc/en-us/articles/13244959138834 For instance tests cannot be executed...

For this reason, I'm suspending the merge to main and we have to wait for Rider support. Of course, after that we can merge, because a lot of work was done.

I can help with autofac issue too. In one of the project we changed approach and used PipelineBehaviors of Mediatr instead of autofac decorators.

bistok commented 8 months ago

@kgrzybek I evaluated about changing to using the functionality on Mediatr, but not do it because that was a fundamental change from the code and the documentation need to be changed. But I agree with you that this will be a good path to follow.

@AlmarAubel you could live with the ICommand and IQuery on buildingblocks? there almost 300 files that need changes to revert this back, can you elaborate a bit exactly what do you loose, becase you can check the derived clases for the contracts in each module only use one contract and not four different contracts.

kgrzybek commented 8 months ago

I upgraded to Rider 2023.3 and it is better. I need to check whether it is production ready.

@bistok We lose type check during command execution and this is important. Commands and Queries are public API of modules. Maybe would be better to do opposite -> applying changes from particular commits (cherry picks?) and do not apply this particular change.

Or maybe based on your pr I can do it myself :)

bistok commented 8 months ago

@kgrzybek I have the change almost ready, running integration test to validate

bistok commented 8 months ago

@kgrzybek I added logging to the test to debug the errors, I leave it or better remove it?

bistok commented 8 months ago

@kgrzybek and @AlmarAubel reverted back, all test executed with no problems.

kgrzybek commented 8 months ago

Thanks @bistok , I will check this on weekend.

Btw - do you know why CI does not work? There is /home/runner/work/_temp/8843f17d-027e-4cda-bf02-f5f5b946831e.sh: line 1: ./build.sh: Permission denied, it looks like gh issue.

bistok commented 8 months ago

yes I found this: https://aileenrae.co.uk/blog/github-actions-shell-script-permission-denied-error/

I thinks is because I checked in the file on windows

I ran the command on MAC and upload the file again, try

kgrzybek commented 8 months ago

Great work, @bistok 🥇 . Thank you very much. Everything looks fine, merged 👍

I will add you both to main contributors page.