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

update to dotnet 6 #90

Merged
merged 8 commits into from
Dec 3, 2021
Merged

update to dotnet 6 #90

merged 8 commits into from
Dec 3, 2021

Conversation

jhgbrt
Copy link
Contributor

@jhgbrt jhgbrt commented Nov 27, 2021

updated target framework in all projects
simplified Program.cs and removed Startup.cs
(as per net5 -> net6 migration guidance)

@xperiandri
Copy link

I don't think that it is a good idea to remove Startup.cs
The simplified template for dummies in .NET 6 is not what a professional Orchard Core project requires.

@jhgbrt
Copy link
Contributor Author

jhgbrt commented Nov 27, 2021

Well, it's a sample intended to demonstrate how OrchardCore Commerce can be used. I would think that especially new developers would expect it to align with the simplified approach (which for better or worse, newcomers will be familiar with in the future).

[opinion]
I believe the bootstrap magic dance between Program and Startup added little to no value. I like the way you now have one place where you can find all bootstrap logic in the new template & philosophy, even for 'advanced' projects.
[/opinion]

Of course, I'll leave it to the project maintainers to apply this PR or not.

SampleWebApp/Program.cs Outdated Show resolved Hide resolved
SampleWebApp/Program.cs Outdated Show resolved Hide resolved
@Skrypt
Copy link
Contributor

Skrypt commented Nov 27, 2021

This will probably break if we multi-target frameworks. I think it is better to keep the startup.cs file.

@jhgbrt
Copy link
Contributor Author

jhgbrt commented Nov 27, 2021

I don't understand your concern. What does the sample web app have to do with multi-targeting?

@Skrypt
Copy link
Contributor

Skrypt commented Nov 27, 2021

We want to be able to test it with the CI by building it with netcoreapp3.1, net5.0, net6.0. And these changes will only work in net6.0 as far as I know? The CI will build every project, sample app included. And I also added functional tests with Cypress in the PR I closed to run it on the CI too. This could be run on every build too.

The new approach in net6.0 is nice but we need to make sure it will pass on the CI. Unless Bertrand wants to only support net6.0. I think some people are already using OrchardCore.Commerce and will then need to revert to some commit version because we don't have versions in here with OrchardCore.Commerce. I think we would need to also add Nuget Packages and everything that comes with it. I don't want to push too much on @bleroy time for doing these things.

@agriffard
Copy link
Member

See OrchardCMS/OrchardCore#10761 for the same concern on the main repo.

@Skrypt
Copy link
Contributor

Skrypt commented Dec 3, 2021

All is good here too. We can merge. I suggest we keep a discussion open about the new way of bootstrapping the app.
Next PR, multi-target.

@bleroy bleroy merged commit 097eaa7 into OrchardCMS:master Dec 3, 2021
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.

None yet

5 participants