Remove PackageGroup

[joshdholtz]

Motivation

Remove unneeded package group from paywall components

Description

PackageGroupComponent was not really needed. It was an extra type of component to think of that didn't provide much value over a StackComponent.

• Removed PackageGroupComponent (and its view and view model)
• PackageComponent now has boolean if its default selected
• PackageComponentViewModel now finds the Package from the package identifier
• New PackageValidator class
  • Injected into recursive view model creator and collects PackageComponentViewModels all the package views
    • Previously this was done in PackageGroupViewModel however, PackageComponents can live anywhere in the paywall
    • Injecting this clas into the whole process is a way to collect all the view models without recursively finding them again later
  • Validate packages (that at least one exists)
  • Finds the default selected package

[tonidero]

LGTM! just a suggestion... Also leaving it to someone with more context on the approach to approve

[tonidero]

Hmm instead of passing the validator around the view models, would it be possible to perform an initial validation before starting to create views/viewmodels? I think it would be cleaner if after we can just assume it's valid. (There might be a reason why that's difficult not really sure, so FFTI).

[joshdholtz]

@tonidero This is what I wanted to do however, it would required traversing the component tree twice (since there is no standard placement for packages)

So I figured if we were already traversing/remapping the components to view models, might as well pull out the information we need at the same time?

It feels gross from a code aspect but felt better in performance? 🤷‍♂️ And maybe I should add this as a comment?

[JayShortway]

Maybe we can abstract this away somehow. What about a wrapper function, e.g. buildInitialState(), that wraps toViewModel() and calls isValid at the end, and returns the root VIewModel + PaywallState?

[joshdholtz]

Maybe we can abstract this away somehow. What about a wrapper function, e.g. buildInitialState(), that wraps toViewModel() and calls isValid at the end, and returns the root VIewModel + PaywallState?

I dooooo like an idea of this! I think I'd like to come back to that a little later though 🤷‍♂️ Once we have things out and have a little more time to breath? Or until this absolutely feels horrible? 😛

[JayShortway]

Yea it's not that much in the way, for sure! I'm okay with keeping it around for now.

[JayShortway]

I like it! Had some comments, mostly around ease of understanding.

[JayShortway]

This can get a default value of PackageValidator() so it almost becomes an implementation detail of the toViewModel() function. This is a usual pattern I think with recursive functions, where the top level call gets a default argument, which it then passes down to every recursive call.

[joshdholtz]

@JayShortway I did think about this but I almost don't want to because it really needs to accept that single high level instance of PackageValidator. I want to make sure that its not accidentally skipped when somebody creates a new view model 😬

I'm kind of keeping it here for somebody adds another container component and needs to pass packageValidator into it and it causes a compile error if its missing 🤷‍♂️ Hopefully that makes sense 😅

[JayShortway]

Yea works for me! I can always get behind a lil more explicitness.

[JayShortway]

Maybe we can abstract this away somehow. What about a wrapper function, e.g. buildInitialState(), that wraps toViewModel() and calls isValid at the end, and returns the root VIewModel + PaywallState?

[JayShortway]

Nice nice!