Refactor FXIOS-8971 Tweaks to redux actions #19823
Conversation
OrlaM
added
the
Do Not Merge ⛔️
|
This change will unblock #19670 |
|
FYi, Bitrise is red due to the warnings, there are 21 and the threshold is 9. https://mozilla.slack.com/archives/CAFC45W5A/p1713286294721309 |
| func displayString() -> String { | ||
| let className = String(describing: Self.self) | ||
| let actionName = String(describing: self).prefix(20) | ||
| return "\(className).\(actionName)" | ||
| } | ||
| } | ||
|
|
||
| public protocol ActionType {} |
There was a problem hiding this comment.
I believe it would be beneficial to use enums for actions, mainly for two reasons. Firstly, the autocomplete feature in Xcode significantly enhances action discoverability. It conveniently allows you to view all options in one place. Secondly, using enums helps ensure that every new action added is handled properly because the compiler will flag an error if any action is overlooked. This approach helps avoid the potential oversight that might occur with a default case in switch statements.
There was a problem hiding this comment.
The objects that conform to ActionType are enums.
There was a problem hiding this comment.
When you switch on ActionType, you loose this capability.
There was a problem hiding this comment.
I may not be understanding. Are you proposing we have one big ActionType enum for all actions in the project?
There was a problem hiding this comment.
Just took a quick look but changes LGTM so far; going to do a deeper dive as soon as I have time. Thanks for digging into this @OrlaM
| FakeReduxAction.counterIncreased(let value), | ||
| FakeReduxAction.counterDecreased(let value): | ||
| return FakeReduxState(counter: value, | ||
| guard let action = action as? FakeReduxAction else { return state } |
There was a problem hiding this comment.
One minor concern: it seems like with this change, reducers/middlewares that are interested in handling several different Action subclasses are going to have to check the class type of the incoming action against every subclass they want to handle 1 or more types for. Not a big deal, but that might lead to some clunky boilerplate of its own since we'll need to have multiple switches against the .actionType for each individual Action subclass, wrapped by if statements. (Hopefully that made sense the way I phrased it)
There was a problem hiding this comment.
I agree this is a concern. It touches on one of the other actions items I have to follow up on and that is how we have broken down and organized our states/middlewares and reducers. They are too big and broad right now, we have some switch statements that are already hundreds of lines long.
Ideally a reducers that wants to reduce actions from more than one Action subclass should be the exception rather than the norm. I'm hoping that will avoid this approach turning into a mess of casting attempts and fallbacks. Open to discussing further if you think that won't be enough.
OrlaM
force-pushed
the
om/FXIOS-8971-redux-actions
branch
from
afa46cc
to
dd36f39
Compare
|
Still a little bit of clean up to do so keeping it in draft for now, but this is now implemented everywhere that uses redux and now compiles. Would love some feedback. I'll comment again once it's fully ready for review. |
Client.app: Coverage: 30.77
Generated by 🚫 Danger Swift against f2d38b4 |
OrlaM
removed
the
Do Not Merge ⛔️
|
This should be ready to go now. |
OrlaM
changed the title
Took another look at the recent commits, it's still looking good to me. |
|
@mattreaganmozilla I'm going to merge this because it's blocking others who want to add new redux actions, plus I'm worried about bugs being introduced by merge conflicts the longer it stays open. Please still give it another pass and leave comments where needed and I'll follow up with any change requests in a new PR. |
|
@Mergifyio backport release/v126 |
✅ Backports have been created
|
* POC for tweaks to redux * Update theme actions * Update fakespot and felt privacy * Update general browser actions * Update tab peek action * More updates * Update tab actions * Fix unit tests * Fix tests * Remove viewUUID * Fix comment * Add missing action handler * Remove TODO * Fix private browsing issue (cherry picked from commit 7032936) # Conflicts: # firefox-ios/Client/Frontend/Fakespot/FakespotViewController.swift
|
@OrlaM Gave this another look and nothing jumped out; I haven't had time for a detailed review of the full diff but tried to give everything a quick once-over and the refactors LGTM. |
* Refactor FXIOS-8971 Tweaks to redux actions (#19823) * POC for tweaks to redux * Update theme actions * Update fakespot and felt privacy * Update general browser actions * Update tab peek action * More updates * Update tab actions * Fix unit tests * Fix tests * Remove viewUUID * Fix comment * Add missing action handler * Remove TODO * Fix private browsing issue (cherry picked from commit 7032936) # Conflicts: # firefox-ios/Client/Frontend/Fakespot/FakespotViewController.swift * Fix conflict * Fix fakespot merge issue --------- Co-authored-by: OrlaM <omitchell@mozilla.com>
📜 Tickets
Jira ticket
Github issue
💡 Description
This is a POC for a tweak to the redux system to try to simplify how we handle actions and associate them with the correct window/view etc.
Previously actions were entirely enum driven, this change shifts it so that actions are classes that contain an action type enum variable within. This allows the action to carry associated data in a simpler way, especially for data that we want to present in all or many cases such as a window ID and a view ID and can easily be expanded to add more if needed in the future.
This removes the need for ActionContext associated values on each of the action enums cases and removes the need to pull the windowUUID var from ever single enum case and the same for future top level variables that would need to be added like viewUUID to fix bugs arising from actions misfiring in places like the normal and private tab panels.
For the sake of the POC I have only updated the redux library and it's tests so that they run successfully. I've updated the active screen section of the client app just as an example but the client app does not currently compile.
📝 Checklist
You have to check all boxes before merging
@Mergifyio backport release/v120)