Skip to content
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

DRAFT: Prototype of exposing hooks for core view manager providers #8326

Closed
wants to merge 2 commits into from

Conversation

rozele
Copy link
Collaborator

@rozele rozele commented Jul 28, 2021

For apps that don't require ABI, expose an API to create IViewManager instances (including those that derive from ViewManagerBase) directly.

Microsoft Reviewers: Open in CodeFlow

@rozele rozele requested a review from a team as a code owner July 28, 2021 15:41
@rozele rozele marked this pull request as draft July 28, 2021 15:41
@asklar
Copy link
Member

asklar commented Jul 28, 2021

For apps that don't require ABI, expose an API to create IViewManager instances (including those that derive from ViewManagerBase) directly.

Microsoft Reviewers: Open in CodeFlow

note that we want to actively discommend using the C++ APIs directly and instead have everyone one the ABI, as this will enable apps/modules/framework to be distributed in binary form. We should be moving more stuff towards ABI not the opposite :)

@rozele
Copy link
Collaborator Author

rozele commented Jul 28, 2021

note that we want to actively discommend using the C++ APIs directly and instead have everyone one the ABI, as this will enable apps/modules/framework to be distributed in binary form. We should be moving more stuff towards ABI not the opposite :)

Totally get it, but this will dramatically reduce the number of differences we have in our fork of RNW native code. The main limitation of the ABIViewManager is it's based off FrameworkElement and not DependencyObject (see #5539). Agree that we should move forward towards this, but this solves a very immediate need for us.

@NickGerleman
Copy link
Collaborator

This change does not seem to affect our own DLL surface, so I'm okay with it.

Plugging directly into internal APIs has the potential to be a footgun, but long term forks kind of pay into that already.

Copy link
Member

@asklar asklar left a comment

Choose a reason for hiding this comment

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

we're having active discussions with you all on how to best enable this scenario in an ABI safe manner, so blocking until we resolve that

@ghost ghost added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jul 29, 2021
@rozele
Copy link
Collaborator Author

rozele commented Aug 3, 2021

Closing for now, we've hopefully worked around all the cases where we needed non-ABI view managers at this time.

@rozele rozele closed this Aug 3, 2021
@ghost ghost removed the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants