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

Make dotnet watch run restart on json and gql changes #50

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

JelteF
Copy link
Contributor

@JelteF JelteF commented Oct 5, 2021

When running with dotnet watch run the binary gets rebuilt and
restarted whenever a .cs file changes. For this project a lot of
behaviour is defined in the config files though. This makes sure the
watch run restarts the program whenever a json or gql file is modified
in the project.

@@ -14,6 +14,11 @@
</Content>
</ItemGroup>

<ItemGroup>
<!-- extends watching group to include .json and .gql files -->
<Watch Include="**\*.json;**\*.gql" Exclude="obj\**\*;bin\**\*" />
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point, I'd imagine we scope this to a given configuration folder instead of a catch all on file type? I'm thinking about some maybe non-restart critical updates to JSON files such as for VSCode/ IDE configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that would make sense. Let's do that once people start running into issues because of this.

@seantleonard
Copy link
Contributor

Looks good.

@JelteF JelteF enabled auto-merge (squash) October 5, 2021 17:14
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

we shouldn't restart the whole DotNet application.
I think we shouldn't do this from Cosmos.GraphQL.Service.csproj

Doing it from the proj file may actually cause a bit of disruption when we develop and locally edit something in the project folder.

IMO, we should park this work till have a better understanding of the requirements of Config update and directory watcher.

@@ -14,6 +14,11 @@
</Content>
</ItemGroup>

<ItemGroup>
<!-- extends watching group to include .json and .gql files -->
Copy link
Contributor

@moderakh moderakh Oct 5, 2021

Choose a reason for hiding this comment

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

we shouldn't restart the whole DotNet application.
I think we shouldn't do this from Cosmos.GraphQL.Service.csproj

Doing it from the proj file may actually cause a bit of disruption when we develop and locally edit something in the project folder.

IMO, we should park this work till have a better understanding of the requirements of Config update and directory watcher.

Copy link
Contributor Author

@JelteF JelteF Oct 5, 2021

Choose a reason for hiding this comment

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

It only does the restart when you run the application with dotnet watch run (note the watch part). This command is meant to restart the application whenever any file changes, so you can easily test out your changes. Withou this changes it only restarts the application when you change a .cs file. Now it does it as well if you change the configs. If you don't like this workflow, you can still use plain dotnet run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, this is separate from any automatic config reloading that we would implement in the application that users would use. That's something we might implement in the future. This is simply correctly configuring dev tooling that we (the devs on this project) can use.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this out and found this useful to enhance my development loop when I'm using dotnet watch. This doesnt affect anything if I'm not doing a watch, so I think this change can go in.

Copy link
Contributor Author

@JelteF JelteF Oct 21, 2021

Choose a reason for hiding this comment

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

@moderakh @jarupatj are you okay with merging this? The IOptionsMonitor definitely looks like more work to implement, and the current approach enhances the dev loop when using dotnet watch. So this PR seems like a net improvement to me. Once we implement real config reloading in the app itself, we will probably remove these lines from csproj. But until then this seems like a good workaround, so I think this should be merged. Let's try not to fall into the trap where perfect is the enemy of good.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is ok with me. Can we have a work item to track this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a workitem here: #67

@jarupatj
Copy link
Contributor

jarupatj commented Oct 8, 2021

You should look at using IOptionsMonitor
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/options?view=aspnetcore-5.0#options-interfaces

@moderakh moderakh self-requested a review October 29, 2021 17:03
Copy link
Contributor

@moderakh moderakh left a comment

Choose a reason for hiding this comment

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

LGTM

When running with `dotnet watch run` the binary gets rebuilt and
restarted whenever a `.cs` file changes. For this project a lot of
behaviour is defined in the config files though. This makes sure the
`watch run` restarts the program whenever a json or gql file is modified
in the project.
@JelteF JelteF merged commit 44a1571 into main Nov 1, 2021
@JelteF JelteF deleted the watch-include-configs branch November 1, 2021 09:51
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