Skip to content

Rename items (elements) used in service reference scenarios #7492

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

Closed
dougbu opened this issue Feb 12, 2019 · 7 comments
Closed

Rename items (elements) used in service reference scenarios #7492

dougbu opened this issue Feb 12, 2019 · 7 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@dougbu
Copy link
Contributor

dougbu commented Feb 12, 2019

This is a pivot on what users see in their projects when using service reference features.

Current items are

<ServiceFileReference Include="SomeFile" ... />
<ServiceUriReference Include="https://SomeUri" ... />
<ServiceProjectReference Include="../SomeProject/SomeProject.csproj" ... />

New elements will be
<ProtobufReference Include="SomeFile" ... />

<OpenApiReference Include="SomeFile" ... />
<OpenApiProjectReference Include="../SomeProject/SomeProject.csproj" ... />

<OpenApiReference Include="SomeFile" SourceUri="https://SomeUri" ... />
<ProtobufReference Include="../SomeProject/SomeProject.csproj" ... />

<OpenApiProjectReference Include="../SomeProject/SomeProject.csproj" ... />
@dougbu dougbu added 1 - Ready enhancement This issue represents an ask for new feature or an enhancement to an existing one area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Feb 12, 2019
@dougbu dougbu added this to the 3.0.0-preview4 milestone Feb 12, 2019
@dougbu dougbu self-assigned this Feb 12, 2019
@dougbu
Copy link
Contributor Author

dougbu commented Feb 12, 2019

Open questions:

  1. Do we use the new item names when calling out to code generation partners? I lean toward "yes" for consistency but that adds more parts that need to change. @glennc do you want to make the call here?
  2. Is the SourceUri case interesting for gRPC too? @shirhatti?
  3. Is the <ProtobufReference Include="../SomeProject/SomeProject.csproj" ... /> case a bit too confusing? Naming (of the extracted document -- just in the Open API case, the C# files, and the the generated class) might be simpler and more consistent if this were something like <ProtobufReference Include="SomeFile" SourceProject="../SomeProject/SomeProject.csproj" ... />. @glennc?
  4. We're leaning toward supporting the (Open API-only) case where the service project does not generate a document on disk when it builds. Will there be an explicit gesture to "refresh" the document a client project extracts? Or, will this be done automatically e.g. on every non-design-time build? @glennc and @mlorbetske I think both of you have opinions here…

@glennc
Copy link
Contributor

glennc commented Apr 1, 2019

We talked about how to handle the upgrade from preview3 to 4 with the new names. What we want is for it to fail hard and fast so that customers know straight away to go rename the element.

@glennc
Copy link
Contributor

glennc commented Apr 1, 2019

Answering open questions:

  1. Yeah, consistency seems good and we have to update them anyway
  2. This doesn't matter anymore due to the naming, we also cut URI support
  3. The project case is just a wildacard for * references
  4. Doesn't matter given the new split.

@dougbu
Copy link
Contributor Author

dougbu commented Apr 3, 2019

From discussions today:

  1. Make the Swagger / OpenAPI project case a true project reference to ensure build ordering
  2. Add metadata to ensure client project doesn't get assemblies / types from the server project
  3. Call a separate project on the web api project to get the actual file list the project wants to surface
    • do this for gRPC too but skip (1) and (2) for that case
    • or, skip project references for gRPC choice

Bottom line: Use what's there and build on it.

@dougbu
Copy link
Contributor Author

dougbu commented Apr 4, 2019

<ProjectReference include = "..\someproject\someproject.csproj" ReferenceOutputAssembly="false" OpenApiReference="true"/>
<ItemGroup>
   <!-- Check in the IDE. Not clear what VS does with such a virtual element? -->
   <ProjectReference Include="@(OpenApiProjectReference)"
        ReferenceOutputAssembly="false"
        Exclude="@(ProjectReference)" OpenApiReference="true" />
</ItemGroup>
<!-- Disable in design-time builds? Definitely requires an <Exec/> to generate code. -->
<!-- See https://github.com/dotnet/project-system/blob/master/docs/design-time-builds.md -->
<Target AfterTarget="ResolveProjectReferences" Name="DoWhatIMean">
  <!-- May also need to _pick_ a supported Config and not assume web api and client projects share a configuration. -->
  <!-- See https://github.com/Microsoft/msbuild/blob/a0817cf07a805ee3f929f19b9336a2d3544cbd84/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1829-L1844 -->
  <MSBuild target="GetMeThatStuff" 
       Project="@(ProjectReference)"
       RemoveProperties="TargetFramework;TargetFrameworks;RuntimeIdentifier" 
       Condition=" '%(OpenApiReference)' == 'true' ">
     <Output items="OpenApiReference" />
  </MSBuild>
</Target>

On the server side, disabling build in old-style (WebApi) projects may require a bit more work. Would need " '$(BuildingProject)' != 'true' " condition there.

@vijayrkn
Copy link

vijayrkn commented Apr 4, 2019

@davkean - Do you think adding something like the above would impact the tree node in VS?

dougbu added a commit that referenced this issue Apr 19, 2019
- #7492
- remove document generation from client project; will be included in Web API project infrastructure (coming soon)

Also
- provide `%(FirstForGenerator)` metadata, #4916
- add `$(OpenApiGenerateAtDesignTime)` property (default `true` for now), #4944
- generate code in `obj` directory by default, #4945
dougbu added a commit that referenced this issue Apr 19, 2019
- #7492
- remove document generation from client project; will be included in Web API project infrastructure (coming soon)

Also
- provide `%(FirstForGenerator)` metadata, #4916
- add `$(OpenApiGenerateAtDesignTime)` property (default `true` for now), #4944
- generate code in `obj` directory by default, #4945
dougbu added a commit that referenced this issue Apr 22, 2019
- #7492
- remove document generation from client project; will be included in Web API project infrastructure (coming soon)

Also
- provide `%(FirstForGenerator)` metadata, #4916
- add `$(OpenApiGenerateAtDesignTime)` property (default `true` for now), #4944
- generate code in `obj` directory by default, #4945
dougbu added a commit that referenced this issue Apr 22, 2019
- #7492
  - remove document generation from client project; will be included in Web API project infrastructure (coming soon)
  - adjust eng/ProjectReferences.props to new project name
  - simplify item de-duplication in `_CreateCompileItemsForServiceFileReferences` target i.e. use `Remove` attribute
    - also handle `.tsx` files in this target
- provide `%(FirstForGenerator)` metadata, #4916
- add `$(OpenApiGenerateAtDesignTime)` property (default `true` for now), #4944
- generate code in `obj` directory by default, #4945
- provide a default `%(CodeGenerator)` value, ##7491 1 of 2 (remainder will come in next milestone)

nits:
- remove a useless `StringBuilder.Append(...)` call
- remove `%(OpenApiProjectReference.SourceProject)` metadata, duplicated `%(OriginalItemSpec)`
- be more consistent about using element syntax for item metadata
@dougbu
Copy link
Contributor Author

dougbu commented Apr 22, 2019

ce8f053

@dougbu dougbu closed this as completed Apr 22, 2019
@dougbu dougbu added Done This issue has been fixed and removed 2 - Working labels Apr 22, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

4 participants