Skip to content

Conversation

davidfowl
Copy link
Member

This reverts commit ad7455c.

People are doing reflection...

This reverts commit ad7455c.

People are doing reflection...
@davidfowl davidfowl requested a review from ericstj May 11, 2021 00:35
@ghost
Copy link

ghost commented May 11, 2021

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented May 11, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

This reverts commit ad7455c.

People are doing reflection...

Author: davidfowl
Assignees: -
Labels:

area-Extensions-DependencyInjection, new-api-needs-documentation

Milestone: -

@eerhardt
Copy link
Member

I’m not sure I follow. If people use reflection, won’t the TypeForwardedTo still work?

Can you give an example the reflection we want to keep working?

@davidfowl
Copy link
Member Author

OData/WebApi#2490

@eerhardt
Copy link
Member

Won’t this break #52363?

@davidfowl
Copy link
Member Author

Won’t this break #52363?

Yep. I'm gonna bundle it with that change after this is merged.

@eerhardt
Copy link
Member

bundle it with that change after this is merged.

I don't get it. Why not just leave the original change in and close this PR? Why revert the change and put it back?

@davidfowl
Copy link
Member Author

davidfowl commented May 11, 2021

Because it's blocking dependency flow to efcore dotnet/efcore#24861

😢

@ericstj
Copy link
Member

ericstj commented May 11, 2021

@davidfowl, is nothing else depending on the type move? In other words, would it have been fine to combine #52363 and ad7455c from the start? If so, I'm fine with reverting so that we can unblock dependency flow.

@ericstj
Copy link
Member

ericstj commented May 11, 2021

To connect the dots for others.

OData is using public reflection to locate ServiceCollectionContainerBuilderExtensions type. They assumed it was in the same assembly as ServiceCollection. That's no longer true after ServiceCollection was moved to abstractions. ServiceCollectionContainerBuilderExtensions cannot be moved down since that would bring down the actual internals of the extensions DI implementation. The fix here is for OData to stop making that assumption, either by not using reflection, or just using reflection to get method and not type. Once OData does that and EFCore is able to pick up a new version, we can move the type.

@davidfowl
Copy link
Member Author

I also made this PR to Odata

@ericstj
Copy link
Member

ericstj commented May 12, 2021

@davidfowl are you planning to merge this or were you going to work around it in efcore?

@davidfowl
Copy link
Member Author

@davidfowl are you planning to merge this or were you going to work around it in efcore?

I'm debating. I feel like I want to work around it since the fix is approved in Odata. I'd like to get them to merge and push something.

@davidfowl
Copy link
Member Author

We're going to work around this in EF and fix Odata.

@davidfowl davidfowl closed this May 12, 2021
@davidfowl davidfowl deleted the davidfowl/revert-type-move branch May 12, 2021 17:05
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants