Windows icon indicating copy to clipboard operation
Windows copied to clipboard

Removed usage of InternalsVisibleTo in all components

Open Arlodotexe opened this issue 1 year ago • 4 comments

This PR:

  1. Removes all usage of InternalsVisibleTo in components in this repo, closes #441.
  2. Copies minor helpers such as ArgumentNullExceptionExtensions, keeping them internal.
  3. Makes public any internal code required to build, mostly around Animations references in Media and Extensions references in Animations.

For 3), it's worth noting that several API surfaces were marked as public which were previously internal. Only the areas that needed to be exposed for other toolkit packages to build on top were updated.

These changes were required for these components to be extended by other components in the toolkit. I'm in favor of publishing them as public for others to build on to the same extent we have, unless we have information on why these were originally made internal and good reason to keep it.

Arlodotexe avatar Jul 02 '24 21:07 Arlodotexe

Tagging @Sergio0694 in on some of these questions, as a lot of the internal APIs are from his Animation library, so he may know more of why they were internal only.

michael-hawker avatar Jul 02 '24 22:07 michael-hawker

I really dislike these changes. What is the actual issue that's caused by using [InternalsVisibleTo]? Can we not only enable it for these two projects?

We're testing these changes for three reasons:

  • https://github.com/CommunityToolkit/Windows/issues/169#issuecomment-2181688627
  • https://github.com/CommunityToolkit/Windows/pull/404
  • ~~https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/195~~

The last reason is dismissed, https://github.com/CommunityToolkit/Tooling-Windows-Submodule/pull/195#issuecomment-2204276472 helped us verify that it was unrelated, and the fix offered by Mano and yourself @Sergio0694 double confirms that.

The first two are still a possible concern. If we can verify that the use of InternalsVisibleTo here isn't uncovering a platform bug (causing #169 or #404), we can dismiss that as well and close this PR without merging.

Arlodotexe avatar Jul 02 '24 22:07 Arlodotexe

I really dislike these changes.

@Sergio0694 To move this along, could you elaborate on this?

The rationale here was to expose the same APIs to consumers that we ourselves are using to build controls.

Since this is code you wrote, we'd pinged you to gather your insights on why these were marked internal so we could align our implementation with our rationale.

Arlodotexe avatar Apr 03 '25 21:04 Arlodotexe

"The rationale here was to expose the same APIs to consumers that we ourselves are using to build controls."

Those APIs are an internal implementation details of our other APIs, and were not meant to be made public. We should make APIs public because they make sense, not just because we need them to make things build. This PR should not be merged.

Sergio0694 avatar Apr 03 '25 21:04 Sergio0694