[Feature] nx conventions on shared dependencies, unit tests with code, dist

[tzuge]

Is your feature request related to a problem? Please describe.

I am looking at reconciling our own dotnet plugin for nx with @nx-dotnet/core. We currently use Directory.Build.props and Directory.Build.target to customize the dotnet project to align to some default nx conventions, e.g. build output to /dist at the repo root, tests/specs next to code (under the same project). As another example, package references could go into Directory.Build.props at the root and be applied to all projects under, which might remove the need for the sync generator.

I'm interested in whether this sort of opinionated setup belongs in this library and how to incorporate it if yes.

Describe the solution you'd like

Clarity on whether we should try to contribute the opinionated workspace configuration and how best to structure it. e.g. parameter on init generator, etc.

Describe alternatives you've considered

We can use @nx-dotnet/core supplemented with our own plugin for the more opinionated templates.

[bcallaghan-et]

We had considered using Directory.Build.props for NuGet packages, as it would be easy to keep all projects on the same package versions, but we decided against it due to how .NET handles package references in the build output. If you were to put all packages in Directory.Build.props and then run dotnet build project-a or dotnet publish project-a, the resulting folder would contain the artifacts of all package references, not just the ones used by Project A. We decided that the cleaner output folder, with minimal artifacts, was worth the extra effort needed to keep all of the projects synchronized.

I am curious how these MSBuild files could allow test files co-located with production files. In my Angular projects, I love the way the tests aren't far from the system-under-test, but I didn't know it could be done with .NET. Can you provide more detail on how that can be done, and what trade-offs might be involved?

As for standardizing all build output in the dist folder, @nx-dotnet/core already does this by modifying the output path within each .csproj file. Personally, I find the direct XML manipulation a bit tedious and hard to test, so I would love to hear how that can be standardized in a single file.

[AgentEnder]

Hey @tzuge! I know we briefly chatted about this in the past, and I had been meaning to follow up on it, but it had been incredibly building. I'd love to help this merger happen. New contributors are always welcome, and new users mean more testing and better results overall.

Original Discussion

Regarding the three concerns directly mentioned here, I'll detail the current background and paths I'd be willing/happy to embrace.

Shared Dependencies

We currently handle this via the sync generator and automatically when adding dependencies via the nuget-reference generator. As @bcallaghan-et mentioned, if we were to drop this down into the Directory.build.props file, then it affects the actual build outputs, which isn't something I want.

Same-directory / Inline tests

This is still something that I think is cool and wouldn't be a bad idea so long as the built artifacts were not affected too much. Even if they are, putting it behind a flag would be welcome. See my comments from our original discussion linked above.

I didn't really think that was feasible at the time. This part is not handled from nx-dotnet, but we could add it as a flag to the app/lib generator maybe? Something like --use-inline-tests

Dist folder output

We currently handle a few things in nx-dotnet via XML manipulation on the individual project files. This isn't the prettiest, but working with these files is something we have to do for project references as well. I'd personally be welcome to ripping out the .csproj manipulation entirely and even for reading it just using some of the dotnet commands (e.g. dotnet list). I don't think we could easily get rid of all of it, but it's undoubtedly one of the messier areas in the codebase and could use a second pass.

One of the two things that use XML manipulation currently is the output path mapping. This isn't great. A consequence of this is that when writing the move generator, we will have to recalculate this path and update it again (see discussion on #406).

The other is adding a prebuild target to the apps that runs a node task using an <exec> task. The whole node task looks something like this:

The output manipulation fits Directory.build.props, and the prebuild target would fit in Directory.build.targets. Honestly, I just wasn't aware of these at the time.

I'd welcome a change that removes the current manipulation stuff and replaces it with creating these two files in the init generator.
There are a few caveats I'd like to mention, though.

• I'd like the new method's current output path to be the result. Looking at your all's current implementation, things are slightly different. The output path should look like $(RepoRoot)/dist/$(ProjectRoot).

[tzuge]

Thanks for the quick response guys. I really appreciate the discussion.

In general the challenge I have is that many of the behaviours are already implemented in this library but with different implementation as you guys are highlighting. It doesn't necessarily make sense to introduce an alternative approach with parameters, since why would a user select A or B when end result is more or less identical? So perhaps the right approach is to figure out what's preferred in each case (at least as a default). Somewhat related, the conventions of nx are rather different from the conventions of dotnet development, e.g. sln files. So there will be some awkwardness in reconciling the two.

Shared Dependencies

I agree the extraneous dependencies in outputs is an issue. There are some equivalents on the javascript side.

1. For node/express apps, I believe nx defaults to having node modules as external. The node modules folder based on the root package.json would similarly be a superset for the monorepo in this case. I'm not sure what the norm is for handling this.
2. For frontend apps, webpack configuration bundles and includes tree shaking, so the output distributable is the minimal required.

dotnet similarly supports both models, and so perhaps we should investigate trimming as the tree shaking equivalent.

Same-directory / Inline tests

I rather like this convention as well, but it is different from how dotnet unit tests normally work. Normally there is a dedicated unit test project that references the project under test, so all unit test dependencies, test files, and assets are kept out of the main project. Not sure if this has changed, but in the past this meant InternalsVisibleToAttribute was necessary to unit test internal classes, etc.

The gist of making this work is that we have conditional inclusion of test references and test files. Currently we use the configuration as the condition, but there's probably a better approach. This means that the assembly created for unit testing is different from an actual deployment assembly. That's not really a good thing, but I believe it is consistent with how jest is configured in nx (i.e. unit tests don't exercise the built bundles).

As a related thing, if handling this in the Directory.Build.props and Directory.Build.targets files, the unit test framework decision probably makes more sense at the repository level; i.e. user would decide at init rather than when generating individual apps/libs.

Dist folder output

If you guys are ok with the inclusion of Directory.Build.props and Directory.Build.targets as part of how the library sets up the workspace, I think it does make sense to handle the paths using this mechanism. It seems aligned to both what those files are intended for and keeping common configuration together in the monorepo.

[AgentEnder]

Hey, happy to move forward w/ Directory.Build.props / Directory.Build.targets. I'll open two issues for that now and link them here.

Shared Dependencies

Most of the official Nx executors to build projects include an option called "generatePackageJson" or similar. This can be used along w/ some manual tweaking to generate a package.json that includes only the deps you need for your project, and then installing from that. The result is that you don't have all the node_modules when publishing.

The webpack/rollup-based executors that rely on tree shaking are more closely tied to what we would see in .NET, where building one project produces an output directory that includes the project's dependencies as well. This is an interesting case, where once again I just wasn't aware that .NET Core supported "trimming" for those purposes.

Perhaps trimming is another thing that we could specify in Directory.Build.props.

Same-directory / Inline tests

RE: Adding a flag, this is the only spot I'd really want to parameterize it. We already have some people wishing that some of our stuff more closely aligned with "the .NET way" of doing things, but others (myself included) think that overall the plugin should follow Nx more closely since its intended for integration in a larger monorepo.

I'm not sure about moving the choice for unit test framework to the workspace level. I agree that it is most likely (and probably preferable) that all projects in the monorepo use the same choice, but at the same time that would be a breaking change from the current setup.

It could be the case that when --use-inline-tests is picked we check if the setup for unit tests is present in Directory.Build.props, and if it is then we do nothing. If it isn't, we prompt w/ enquirer to check which framework to setup in Directory.Build.props.

Handling it this way would mean that we don't create a test project when --use-inline-tests is specified, but that we still give the option if the flag isn't passed in. Then it wouldn't be breaking, but we would need to move prompts for test framework over to enquirer rather than the schema.json as well.

[AgentEnder]

Created issues to track individual portions of this conversation:

• [Feature] Generate Directory.Build.props during init generator #469
• [Feature] Generate Directory.Build.targets during init generator #470

[tzuge]

On the topic of inline tests:

We currently use Configuration == Debug condition to include .spec files and unit test framework references. This has the unfortunately consequence that dotnet test --configuration Release would result in no tests found. Using a msbuild property or environment variable is likely the better approach; the nx plugin executor for running the dotnet tests can include the appropriate property or environment variable.

Note this issue in dotnet sdk: dotnet/sdk#23198 as where they land could impact the appropriate way for the executor to run the test command.

[AgentEnder]

I wonder if dotnet msbuild -target:Test may be better, since that would allow using the msbuild props

[tzuge]

Does that mean changing the test executor to use dotnet msbuild instead of dotnet test and potentially include the mstest execution as part of a task in the target?

So far it seems like an environment variable might be the most straightforward way.
dotnet test -e TEST_RUN=true

In csproj / Directory.Build.props

<!-- Remove the unit test files when env not set -->
<ItemGroup Condition="$([System.Environment]::GetEnvironmentVariable('TEST_RUN')) != 'true'">
  <Compile Remove="**/*.Spec.cs" />
</ItemGroup>

It would require the test executor to include this extra parameter when running dotnet cli in case of inline tests, and dotnet test would not directly work against these project (since there is this magic env var involved). Currently the arg passthrough doesn't appear to work for single - arguments and dotnet test gets executed with --e instead.

An additional detail I discovered is that for lib projects, a file with Main needs to be scaffolded for the entry point into the 'test' assembly. This could be in a *.spec/test.cs for the same conditional inclusion.