-
Notifications
You must be signed in to change notification settings - Fork 39
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
Setting up dynamic sample page loading #12
Conversation
Still needs to be tested under .NET Native and with the IL Linker enabled in WebAssembly, there's a good chance it'll tree-shake the sample pages out of the final binaries. |
using CanvasLayout.Sample; | ||
using CommunityToolkit.Labs.Core.Attributes; | ||
|
||
[assembly: ToolkitSample(sample: typeof(SamplePage))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so if they add a new page they'd have to add another copy of the attribute here too, eh? 🤔
Wondering if it makes sense to just use a T4 template or a source generator instead. We can have it crawl for the sample pages at build time and spit out a common class based on an interface or something that will be easy to find from each assembly instead? Then the developer doesn't have to attribute anything to get started (we just grab anything that's a Page
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was assuming each sample project contains only one sample page. It'll be included as part of the template, pointing to a blank sample page. The developer won't need to add it themself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michael-hawker Do we want a sample project to contain more than one sample page? If so, I can look at source generators instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd see the sample project having multiple potential pages show casing different pieces of a control/helper. It's the biggest weakness of our sample app today (and the mess that is samples.json
).
Added @Sergio0694 to the repo as he's just dug-in to the latest incremental source generator stuff and can help us get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the kind of API/metadata that each project would need to expose to the sample app? If you wanted each sample page to be mapped to an assembly attribute, that would be easy to do with a source generator, yeah (you could either just add an attribute for all pages, or eg. all pages with a given interface, or all pages with an attribute on them, or whatever). Could you elaborate on the kind of workflow the sample app would do to look for and load these samples from referenced projects? Because eg. if it will just look for assembly attributes to find the list of sample pages and then load them up, then at that point couldn't it just simply go through all exported types directly? 🤔
What I'm saying is I'm not seeing how a source generator would necessarily be worth the extra complexity in this scenario, from what I'm seeing here, especially since a sample app doesn't really care about extra startup performance here that much. Happy to help once I have a better understanding of the infrastructure you're trying to setup 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sergio0694 As mentioned in the PR description,
We could have placed the attribute on the page itself, but iterating through every type in every loaded assembly is much slower, and we can still template the attribute declaration for the user.
Since we'll be running in WebAssembly under Uno (where reflection is very slow) and letting the user build and run each sample individually in a Codespace, we want to keep reflection to a bare minimum and keep startup time as fast as possible, while still pulling in the sample pages dynamically. Source generators would let us do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you actually checked that this is an issue though? Looking up attributes is still reflection. I would just make this work by going through all exported types first, and then consider using a generator only if after doing a manual check with attributes first you can actually verify that you get a noticeably faster startup performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I went with the fastest and simplest solution first. We can give runtime reflection a shot, I have some ideas to minimize the impact.
If it ends up being too slow in a codespace, or .NET Native or the WASM IL linker stops playing nice and starts tree-shaking the sample pages out without a hard reference, we'll have to set up source generators to do the reflection at build time and add everything to a static class we can reference at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sergio0694 it won't be a problem at the individual experiment level, but when we correlate all the pages from all the samples it could get pretty large. This is a larger issue with how the samples.json
loads today and does a bunch of metadata reading reflection at runtime vs. if we can just grab description attributes and categories at build time and effectively just dump an already serialized object constructed version out to C# code that we can load very quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and implement the reflection version of this, if we want to switch to source generators later it shouldn't affect the templates or existing samples.
@Arlodotexe having trouble building this on my machine (UWP or WASM for the specific experiment solution): If I run the specific UWP head for the experiment from the All Experiments it works though. |
@michael-hawker Maybe try doing a clean from VS or a hard clean via |
@Arlodotexe that's how I started out. 🙂 I think if you built the All Experiments first it may have dropped everything the individual one needed. If you do a git clean and then try and run the individual solution version it works for you? Edit: (Going back to the single experiment it builds now that I've built the all experiments, but then wasm fails and UWP is giving me an unhandled exception on start for some reason now.) |
Pushed my branch rebased on top of yours here: https://github.com/michael-hawker/Toolkit-Labs-Test/tree/llama/canvaslayout-dynamic Literally no exception info in the UWP app for me about why it's dying during initialization, debugger timing out for me trying to get exception text and there's no exception being caught beforehand with everything turned on... so weird. |
Common/CommunityToolkit.Labs.Core/Attributes/ToolkitSampleAttribute.cs
Outdated
Show resolved
Hide resolved
Common/CommunityToolkit.Labs.Core/Attributes/ToolkitSampleAttribute.cs
Outdated
Show resolved
Hide resolved
Sub-category could probably just be a string, we'll be able to see during review what things are and decide if it should belong elsewhere. I feel top-level categories being an enum to help initial buckets is good though. |
@michael-hawker I gave it a good think, and went with using a second enum for all subcategories in a1089ea. It'll give us the flexibility to use a subcategory under multiple categories (e.g. |
@michael-hawker @shweaver-MSFT @XAML-Knight Any additional comments? I think we're finished with this one, I'd like to get it merged 🙂 |
Looks good to me. I ran into another mysterious build-directory-is-read-only build issue, but once I cleaned that up, was up and running with my WebAssembly. |
@shweaver-MSFT think we can merge this and then update the template? I'm going to work on re-basing my CanvasLayout branch to add substance here too. |
@michael-hawker works for me |
@Arlodotexe did you want to add the project reference issue we found after here so when we update the template we won't forget it there too (assuming it doesn't already have it)? |
…namic-sample-loading Setting up dynamic sample page loading
This PR implements dynamic sample page loading, as detailed here. Closes #1.
Breakdown:
CommunityToolkit.Labs.Core
project - referenced by all non-library projects and contains the custom attribute needed to declare and dynamically pull in sample pages. Tried looking for an attribute we could use that's already built into .NET, but found none.ToolkitSampleAttribute
andToolkitSampleMetadata
- decorate aPage
with this in a sample project to register it as a toolkit sample.ToolkitSampleAttribute
.We only have 1 sample project right now, so the multi-sample view can't be triggered. Here's the single-sample view running in both UWP and WASM.
New single-sample solution structure: