Use dependency context from all application parts when compiling views #4672
Conversation
| { | ||
| public void PopulateFeature(IEnumerable<ApplicationPart> parts, MetadataReferenceFeature feature) | ||
| { | ||
| var currentAssembly = GetType().GetTypeInfo().Assembly; |
There was a problem hiding this comment.
The test site is set up so that the only AssemblyPart that was added was for the ClassLibrary that it was referencing. I added this so as to not perturb the arrangement.
|
⌚ |
| } | ||
|
|
||
| var libraryPaths = new HashSet<string>(StringComparer.OrdinalIgnoreCase); | ||
| foreach (var part in parts.OfType<AssemblyPart>()) |
There was a problem hiding this comment.
You should not be casting to AssemblyPart here. Make a new interface that returns a DC and cast to that.
This way a user can plug in a custom part that returns an arbitrary DC if desired.
There was a problem hiding this comment.
Or you can make the interface return metadata references directly... not sure which is better
There was a problem hiding this comment.
We'd have to make Mvc.Core depend on Roslyn \ DependencyModel? Is that fine by you?
There was a problem hiding this comment.
DependencyModel I'm not concerned about because the APM already does.... Roslyn we shouldn't do.
|
⌚ |
pranavkm
force-pushed
the
prkrishn/read-more-app-parts
branch
3 times, most recently
from
94d42cd to
8651e71
Compare
|
🆙 📅 |
| /// <summary> | ||
| /// Exposes a <see cref="DependencyContext"/> from an <see cref="ApplicationPart"/>. | ||
| /// </summary> | ||
| public interface IDependencyContextProvider |
There was a problem hiding this comment.
Sorry in advance for being a jerk.
How hard is a DC to construct? Would it be better to expose a limited set of the stuff it provides from this interface instead?
I'm just asking the question. I feel like this is a good starting point at least.
There was a problem hiding this comment.
It's not the nicest API to work with. See https://github.com/aspnet/Mvc/pull/4672/files/8651e71758e11828e66b9175e999f938fb6a3c2e#diff-c1746f8559d5a5341fe8a1b87c65d5daL304. We could boil this down to reference paths or push the CLI to make this API easier to construct.
cc @pakrym \ @davidfowl
There was a problem hiding this comment.
@pranavkm: How would you like API to look like?
There was a problem hiding this comment.
Not entirely sure. Maybe a utility method that constructs it given reference paths?
There was a problem hiding this comment.
@pranavkm that doesn't seem entirely awful
There was a problem hiding this comment.
Yeah that might be the best, and that gets to the heart of what you might actually want to do.
There was a problem hiding this comment.
@pakrym where does CompileLibrariesProvider live? Is that a suggestion?
There was a problem hiding this comment.
@javiercn Yes, I suggest to create ICompileLibrariesProvider interface instead of IDependencyContextProvider because MVC only uses CompilationLibraries from DependencyContext and in this case no new API is required to simplify dependency context creation.
There was a problem hiding this comment.
I believe the interesting part of CompileLibraries is https://github.com/dotnet/cli/blob/rel/1.0.0/src/Microsoft.Extensions.DependencyModel/CompilationLibrary.cs#L39 that we use on https://github.com/aspnet/Mvc/pull/4672/files#diff-ab147a7292f31e46ebc6b39f1df7480bR49
Maybe we can have our own DTO with the interesting parts of compilelibrary and the resolved paths? I think that strikes a good balance. I think something like ApplicationReference { Name, Version and Paths } will be enough
There was a problem hiding this comment.
Wouldn't it basically be the same type at that point?
Edit: We really don't need the type at all that point. We could simply expose a sequence of reference paths (I suggested that earlier).
pranavkm
force-pushed
the
prkrishn/read-more-app-parts
branch
from
8651e71 to
19d5d1c
Compare
|
🆙 📅 |
|
|
when 
|
Noted a small typo - GetCompilatonLibraries should be GetCompilationLibraries :) |
|
Nice catch @gdau |
pranavkm
force-pushed
the
prkrishn/read-more-app-parts
branch
from
19d5d1c to
18124db
Compare
|
Ping @rynowak |
| /// <summary> | ||
| /// Gets the sequence of <see cref="CompilationLibrary"/> instances. | ||
| /// </summary> | ||
| IReadOnlyList<CompilationLibrary> GetCompilationLibraries(); |
There was a problem hiding this comment.
ICompileLibrariesProvider <-> GetCompilationLibraries
The nouns aren't really the same between these. Should it be ICompileLibrariesProvider?
There was a problem hiding this comment.
Did you mean ICompilationLibrariesProvider?
There was a problem hiding this comment.
Why are we exposing this as a public API?
There was a problem hiding this comment.
The intent is to allow users to add additional compilation libraries
There was a problem hiding this comment.
Show me a sample of that? Why wouldn't they just add a metadata reference?
There was a problem hiding this comment.
Why wouldn't they just add a metadata reference?
You can ask this question about 100% of the application parts feature, but we still lots of requests for something like this and we still ended up building it. You can solve all of the problems that application parts solve by just referencing everything from one project, but users have come up with plenty of cases where they don't want to do that.
As @pranavkm said this is here so you can author a custom part and have control over which assemblies are included as references. Providing interfaces for the various capabilities is part of the design of parts.
Why would you do this? Well I suppose you could use it to work around the bug we have where a .csproj isn't part of the dependency graph 
|
|
Fixes #4498