Upgrade to run .net8 and opinionated changes #27
Conversation
- No need to do custom check for args length in Program as it's already being done in `RunWithGraphQLCommands` - Move contact directive to schema configuration vs custom schema object - Move common graph builder services to separate extension so it can be reused in test case setup
There was a problem hiding this comment.
This is great, thank you for the contribution! I think overall we only need 2 small tweaks:
- We need to ensure that new users are prompted where to go when the server starts up. I thought the simplest method was to just to have the GraphQL explorer experience be the default for whatever url is displayed by
dotnet run. In one of my comments I have a code snippet for configuring banana cake pop to do just that. Another option would be to have a console output after the server starts up that displays the URL for the user to navigate to. - All
rovertemplates are designed to work with simple deployments like Railway. These typically require the service listens on the$PORTenvironment variable which is set in their container framework. We need to keep the code that enforces this to avoid breaking the deployment template in Railway. I have a comment that has a code snippet in there.
You already put in time to get this PR in place and if you don't have the time for the two tweak above, I'm happy to push those changes in. I'll give this comment a couple days and if you haven't gotten back to it yet, I'll make the tweaks and merge.
Thank you again for this contribution ❤️, love the opinions you're adding to the template.
| .AddHttpRequestInterceptor<RouterAuthInterceptor>(); | ||
|
|
||
| var port = Environment.GetEnvironmentVariable("PORT") ?? "4001"; |
There was a problem hiding this comment.
We need to make sure that the template will listen on the PORT environment variable for deployments like Railway.
There was a problem hiding this comment.
Maybe simplify this to:
if (Environment.GetEnvironmentVariable("PORT") != null)
builder.WebHost.UseUrls($"http://*:{Environment.GetEnvironmentVariable("PORT")}");
| var app = builder.Build(); | ||
|
|
||
| app.MapGraphQL(); | ||
| var bananaCakePop = app.MapBananaCakePop("/"); |
There was a problem hiding this comment.
Since this is a template for a new project, I think we should have the / route redirect to the GraphQL explorer experience. For a new user getting started, they might not know to visit the /graphql endpoint to explore the API and it would be good to add that line but make it simpler:
app.MapBananaCakePop("/")
.WithOptions(new GraphQLToolOptions() { GraphQLEndpoint = "/graphql" });
| _ = app.RunWithGraphQLCommandsAsync(args); | ||
| else | ||
| app.Run(); | ||
| app.RunWithGraphQLCommands(args); |
There was a problem hiding this comment.
I love the opinionated launchSettings. I wanted this but wasn't quite sure of the current standards, lots of dotnet releases since I was with Xamarin.
RunWithGraphQLCommandsASPNETCORE_URLS