Skip to content
This repository was archived by the owner on Apr 8, 2020. It is now read-only.

Use Directory.Build.props/targets #1235

Merged
merged 2 commits into from
Aug 30, 2017
Merged

Use Directory.Build.props/targets #1235

merged 2 commits into from
Aug 30, 2017

Conversation

natemcmaster
Copy link
Contributor

We've started using Directory.Build.props/targets across aspnet repos to reduce the duplication between csproj files, and reduce the amount of boilerplate code required to add new projects and tests.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Thanks Nate! Mostly this looks great.

As per my inline comment, could we go back to having each of the src projects declare its own values for the following independently?

  <ItemGroup>
    <None Remove="node_modules\**\*" />
    <EmbeddedResource Include="Content\**\*" />
  </ItemGroup>

  <Target Name="PrepublishScript" BeforeTargets="PrepareForPublish" Condition=" '$(IsCrossTargetingBuild)' != 'true' ">
    <Exec Command="npm install" />
    <Exec Command="node node_modules/webpack/bin/webpack.js" />
  </Target>

Apart from that this looks good to me!

</PropertyGroup>

<ItemGroup>
<None Remove="node_modules\**\*" />
Copy link
Member

Choose a reason for hiding this comment

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

I'm not keen on this or any of the following lines in this csproj being moved out into Directory.Build.props, because:

  1. It's hard to reason about which node_modules or Content files are being included/excluded when the paths aren't even relative to the file that's referencing them
  2. It makes an assumption about conventions for inclusion/exclusion and prepublishing always being the same across every project in src, which isn't guaranteed. Although they might all happen to follow the same conventions today, there's no reason to believe that should always be so. Not all of them should necessarily have a Content directory, use node_modules, etc.

So I would prefer not to deduplicate these values, but rather keep the src projects equivalently decoupled as they were before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I'll revert this piece of the change.

@natemcmaster
Copy link
Contributor Author

🆙 📅

@natemcmaster
Copy link
Contributor Author

I think I address your feedback, so I'm going to merge this so I can get #1236 in too. If there was anything I missed, let me know and I'll fix it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants