Fix swallowing original error

[IeuanWalker]

Had an issue with release builds, where testers where reporting a specific page crashing, when I looked at the logs it was pointing to the fact that a service was missing from the DI container -

But everything was working fine when debugging. When I turned on trimming for the debug build I was able to replicate the issue, but was still getting the same error pointing to a service missing.

I had to clone PageResolver to be able to see the causing issue, which pointed to an issue with XAML accessing a property of an array using an index.

This PR is to return the original error, rather than the fallback attempt, which then results in the correct error being logged -

[matt-goldman]

Thanks for catching this. Sorry for the delay, I'm not 100% sure about this change as it moves from catching an explicitly defined error to catching a generic error instead. I think the problem here is that this masks an exception further down the chain that gets bubbled up here for some reason. Let me give some thought to potential alternative ways of catching this. Otherwise this might need a flag as the MissingMemberException is expected/anticipated behaviour in some scenarios, and it looks like this change will throw that exception irrespective of whether there's a deeper exception or not.

[IeuanWalker]

@matt-goldman no prob, i did do it as a AggregateException orignally, so nothing will be missed. But the logging solution I'm using wasn't handling AggregateException that well and was missing a lot of details.

Will have another play when I get back into Monday.

[IeuanWalker]

@matt-goldman Sorry been pretty busy with work.

Not really sure where to go with this tbh, I only see 3 possible solutions -

• The current PR where it rethrows the original error
  • I know you mention that in some use cases this exception is expected/anticipated behaviour in some scenarios, i wasn't aware of that, thought this was just a final attempt before giving up. In that case this solution would mask the error from the second call to CreateInstance.
• Throw an AggregateException if both fails, i.e. -

private static Page CreatePageUsingViewModel<T>(IServiceProvider serviceProvider, object viewModel) where T : Page
    {
        try
        {
            return ActivatorUtilities.CreateInstance<T>(serviceProvider, viewModel);
        }
        catch (MissingMemberException ex)
        {
            try
            {
                return ActivatorUtilities.CreateInstance<T>(Resolver.GetServiceProvider());
            }
            catch (Exception ex1)
            {
                throw new AggregateException(ex, ex1);
            }
        }
    }

• Add a trace line, i.e.

private static Page CreatePageUsingViewModel<T>(IServiceProvider serviceProvider, object viewModel) where T : Page
    {
        try
        {
            return ActivatorUtilities.CreateInstance<T>(serviceProvider, viewModel);
        }
        catch (MissingMemberException ex)
        {
            Trace.WriteLine(ex);
            return ActivatorUtilities.CreateInstance<T>(Resolver.GetServiceProvider());
        }
    }

My personal preference would be the AggregateException as no information is lost doing it that way.
Trace line would require the developer to notice the error in the output logs.

---

And just to comment on this -

Otherwise this might need a flag as the MissingMemberException is expected/anticipated behaviour in some scenarios, and it looks like this change will throw that exception irrespective of whether there's a deeper exception or not.

It only throws it if both CreateInstance calls fails, and there is nothing stopping the second call from also throwing MissingMemberException so wont be a change of behaviour, unless I'm missing something.

[matt-goldman]

Hi @IeuanWalker thanks for your patience on this! And my apologise too; I've been swamped and also not come back to this until now.

You’re not missing anything, I just didn’t articulate my concern clearly. The MissingMemberException is expected in some scenarios (I added this when I made the change to allow params that can be matched to either the page or the ViewModel), but when the fallback also fails due to a real misconfiguration, that’s the part that gets lost in the original implementation. With the initial version of the PR, the second error (from the fallback) is what gets masked.

I think your latest suggestion (rethrowing an AggregateException if both attempts fail) is the cleanest and most effective approach as it keeps the intent clear while surfacing the real issue. (And it's better than the clunky version I came up with, creating a nullable Page and assigning it in two try/catch blocks, returning if not null and then returning the aggregate exception if execution reaches the end).

Happy to go with the AggregateException approach you've suggested. I'll merge it in once you push that version.

[IeuanWalker]

@matt-goldman ahh cool, just committed the changes :)

[matt-goldman]

Lots of formatting changes - need an .editorconfig!

LGTM