Add FXIOS-9301 [Microsurvey] Redux proposed enhancements

[cyndichin]

📜 Tickets

Jira ticket
Github issue

💡 Description

Update redux changes to microsurvey

• Remove associated value
• Move dependency to be resolved by app container; add missing telemetry call
• Add guard so that actions are only applied to the specific windowUUID

📝 Checklist

You have to check all boxes before merging

• Filled in the above information (tickets numbers and description of your work)
• Updated the PR name to follow our PR naming guidelines
• Wrote unit tests and/or ensured the tests suite is passing
• When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
• If needed, I updated documentation / comments for complex code and public methods
• If needed, added a backport comment (example @Mergifyio backport release/v120)

[cyndichin]

@OrlaM For demoing purposes, I added another commit to discuss using payload instead of our current implementation. The commented out code is to demonstrate what we are doing today. Currently, a lot of actions tend to use a singular built-in object types that we can pull from the payload (like we show here with MicrosurveyModel.

I was curious in whether we also discussed a simple payload with Any? type. The downside I see is that we may need to check the type as an extra step, but already do a lot of checks with the action types as well. For complex payloads, we can create a separate struct to associate with that specific action.

struct CloseTabPayload {
   let panelType: panelType,
   let tabUUID: tabUUID
} 
let payload = CloseTabPayload(panelType: panelType,
                                            tabUUID: tabUUID)
let action = TabPanelViewAction(windowUUID: windowUUID,
                                            actionType: TabPanelViewActionType.closeTab, 
                                            payload: payload)

Let me know what you think! If this doesn't work, I'll go ahead and revert this commit and follow the conventions we have currently. Still investigating on generics, but that'll be separate from this PR if there's a good solution.

[cyndichin]

@tusharC95 @DanielDervishi

[mattreaganmozilla]

In some ways this is sort of bringing us back to what we had previously, except that the payloads were previously concrete subclasses of ActionContext and passed in as associated values. But we had used a similar idea of leveraging the benefits of inheritance in the custom payloads rather than having all of the properties declared on the (new) action classes.

[cyndichin]

@mattreaganmozilla @OrlaM I am going to open a separate PR for the redux cleanup, so that it doesn't get blocked by these proposed enhancements. And will rebase this PR once that gets merged in.

[OrlaM]

Sorry about the delay getting back to this, it took me some time to think it through.
This is great work and it reads much better for simple examples but I don't think we should move forward with it because it is likely to scale with the same issues we have just moved away from. The use of Any? here adds more confusion to the mix and adds back a tonne of verbosity in anything but basic use cases. If we have objects defined for each action's payload we end up with a bunch of objects and it can be very confusing on which one to use in which scenario especially since the expected type is any. For example look at what the TabDisplayAction enum used to look like back when we were using ActionContext subclasses passed as associated values.
https://github.com/dataports/firefox-ios/blob/f32732ab3c13d97f3e6b677d31bdee7f6b03f042/firefox-ios/Client/Frontend/Browser/Tabs/Action/TabPanelAction.swift
This is probably the worst offender for it but it shows how quickly and easily this can get messy.
I do encourage continuing to think about the problem tho because we do have a bunch of ambiguity in how the payload is current handled, which fields are relevant for which action type etc, so I'm all for trying to experiment to find different approaches.

[mergify]

This pull request has conflicts when rebasing. Could you fix it @cyndichin? 🙏